From d8579b76736e242a30f954c82275d34b2661df88 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Jun 2022 20:10:37 +0200 Subject: [PATCH 1/8] Uncomment mbedtls_asn1_write_mpi tests with leading 1 bit mbedtls_asn1_write_mpi() correctly handles the sign bit, so there's no reason not to test that it's handled correctly. Fix copypasta in test data that was commented out. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.data | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_asn1write.data b/tests/suites/test_suite_asn1write.data index f844d4844..50cc1cf7e 100644 --- a/tests/suites/test_suite_asn1write.data +++ b/tests/suites/test_suite_asn1write.data @@ -100,11 +100,11 @@ mbedtls_asn1_write_mpi:"01":"020101" ASN.1 Write mpi 0x7f mbedtls_asn1_write_mpi:"7f":"02017f" -#ASN.1 Write mpi 0x80 -#mbedtls_asn1_write_mpi:"7f":"02020080" +ASN.1 Write mpi 0x80 +mbedtls_asn1_write_mpi:"80":"02020080" -#ASN.1 Write mpi 0xff -#mbedtls_asn1_write_mpi:"7f":"020200ff" +ASN.1 Write mpi 0xff +mbedtls_asn1_write_mpi:"ff":"020200ff" ASN.1 Write mpi 0x100 mbedtls_asn1_write_mpi:"0100":"02020100" @@ -112,8 +112,8 @@ mbedtls_asn1_write_mpi:"0100":"02020100" ASN.1 Write mpi, 127*8-1 bits mbedtls_asn1_write_mpi:"7f7b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8":"027f7f7b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8" -#ASN.1 Write mpi, 127*8 bits -#mbedtls_asn1_write_mpi:"e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8":"028180e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8" +ASN.1 Write mpi, 127*8 bits +mbedtls_asn1_write_mpi:"e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8":"02818000e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8" ASN.1 Write mpi, 127*8+1 bits mbedtls_asn1_write_mpi:"108446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c":"028180108446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c" @@ -121,8 +121,8 @@ mbedtls_asn1_write_mpi:"108446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807 ASN.1 Write mpi, 255*8-1 bits mbedtls_asn1_write_mpi:"7bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c":"0281ff7bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c" -#ASN.1 Write mpi, 255*8 bits -#mbedtls_asn1_write_mpi:"fbd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c":"0282010000fbd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c" +ASN.1 Write mpi, 255*8 bits +mbedtls_asn1_write_mpi:"fbd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c":"0282010000fbd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c" ASN.1 Write mpi, 256*8-1 bits mbedtls_asn1_write_mpi:"7bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c89":"028201007bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c89" From 0ab804a794f2a92b3d7d0d08b60ac7c9a41d233a Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Jun 2022 20:12:25 +0200 Subject: [PATCH 2/8] Fix mismatch between test data and test description Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1write.data b/tests/suites/test_suite_asn1write.data index 50cc1cf7e..01064f5f0 100644 --- a/tests/suites/test_suite_asn1write.data +++ b/tests/suites/test_suite_asn1write.data @@ -116,7 +116,7 @@ ASN.1 Write mpi, 127*8 bits mbedtls_asn1_write_mpi:"e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8":"02818000e77b16e05c1537de7c41cef1a0985d6a3ced98aec28e091874cbad6b5e40a5c956258f18861c28bed8ba808259339ee34b2e509c4080149474d5d5b86093f90c475a6443fc87e1a293d4151be625d652f1c32a00a018bba10c8a2ae5b2b0ee4be64e053dce9d07ec7919526c9dfcf2ec9fc3db485caa8e5a68a2cd0a427de8" ASN.1 Write mpi, 127*8+1 bits -mbedtls_asn1_write_mpi:"108446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c":"028180108446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c" +mbedtls_asn1_write_mpi:"018446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c":"028180018446d68934cc1af23c4cd909884d4bd737a1890e12f5ef8bf3d807d72feffa63c0bf2633345f8b8418d144617c871a7a0277ac0150eed4b3db7f9dff21114cd0d7f282400f03c931cb00c367550e374a1ed3762a1801ca714cfc8d5aac69707ca81e0661400ed0014d97cba48f94d835dd681fc3053c51958afbf7583cf49c" ASN.1 Write mpi, 255*8-1 bits mbedtls_asn1_write_mpi:"7bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c":"0281ff7bd1913fcfb652896209ad3e62f5d04a8dfc71eb1698543c52200bd7bbf3c11dd9ff57c299a2f4da172b3d5bd7e29affddf8859be7d50a45537a0df15b17af603d18803fd17134847cba78d83e64bf9fee58364d6124add0541da7bad331cd35fb48186a74bc502ddb967602401c0db02b19e5d38f09e8618fa7f6a1a3f738629baffdc63d9d70d396007d943fd64ae696e5b7e88f2c6d6ec322b461dbddd36efa91d990343b66419cf4832a22dc9ad13021185a1bf007989a50ba3bfd1152b8db899482d3ed498d1b9fae243a3cdae9530d8b29fdb684f70cdc0c9b8527265312603b405e67d59d4b1d654ddc3b7fd5515acb32440dc80903c8474a2c136c" From c9a30fba745072f4475f4766bc6c029705a6edb9 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Jun 2022 20:12:45 +0200 Subject: [PATCH 3/8] Add MPI write tests when the MPI object has a leading zero limb Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.data | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/suites/test_suite_asn1write.data b/tests/suites/test_suite_asn1write.data index 01064f5f0..f2c32ec81 100644 --- a/tests/suites/test_suite_asn1write.data +++ b/tests/suites/test_suite_asn1write.data @@ -100,9 +100,15 @@ mbedtls_asn1_write_mpi:"01":"020101" ASN.1 Write mpi 0x7f mbedtls_asn1_write_mpi:"7f":"02017f" +ASN.1 Write mpi 0x7f with leading 0 limb +mbedtls_asn1_write_mpi:"00000000000000007f":"02017f" + ASN.1 Write mpi 0x80 mbedtls_asn1_write_mpi:"80":"02020080" +ASN.1 Write mpi 0x80 with leading 0 limb +mbedtls_asn1_write_mpi:"000000000000000080":"02020080" + ASN.1 Write mpi 0xff mbedtls_asn1_write_mpi:"ff":"020200ff" From 321a08944b9f47a52d4f6f683e356f2bc215c3f8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Jun 2022 20:13:33 +0200 Subject: [PATCH 4/8] Fix bug whereby 0 was written as 0200 rather than 020100 0200 is not just non-DER, it's completely invalid, since there has to be a sign bit. Signed-off-by: Gilles Peskine --- ChangeLog.d/asn1write-0-fix | 2 ++ library/asn1write.c | 5 +++++ tests/suites/test_suite_asn1write.data | 7 +++++-- 3 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 ChangeLog.d/asn1write-0-fix diff --git a/ChangeLog.d/asn1write-0-fix b/ChangeLog.d/asn1write-0-fix new file mode 100644 index 000000000..2e01244f8 --- /dev/null +++ b/ChangeLog.d/asn1write-0-fix @@ -0,0 +1,2 @@ +Bugfix + * Fix mbedtls_asn1_write_mpi() writing an incorrect encoding of 0. diff --git a/library/asn1write.c b/library/asn1write.c index 2110052d5..053dbb669 100644 --- a/library/asn1write.c +++ b/library/asn1write.c @@ -133,6 +133,11 @@ int mbedtls_asn1_write_mpi( unsigned char **p, const unsigned char *start, const // len = mbedtls_mpi_size( X ); + /* DER represents 0 with a sign bit (0=nonnegative) and 7 value bits, not + * as 0 digits. We need to end up with 020100, not with 0200. */ + if( len == 0 ) + len = 1; + if( *p < start || (size_t)( *p - start ) < len ) return( MBEDTLS_ERR_ASN1_BUF_TOO_SMALL ); diff --git a/tests/suites/test_suite_asn1write.data b/tests/suites/test_suite_asn1write.data index f2c32ec81..6fc668f53 100644 --- a/tests/suites/test_suite_asn1write.data +++ b/tests/suites/test_suite_asn1write.data @@ -91,8 +91,11 @@ mbedtls_asn1_write_enum:0x12345678:"0A0412345678" ASN.1 Write enum 2147483647 mbedtls_asn1_write_enum:0x7fffffff:"0A047fffffff" -#ASN.1 Write mpi 0 -#mbedtls_asn1_write_mpi:"00":"020100" +ASN.1 Write mpi 0 (null) +mbedtls_asn1_write_mpi:"":"020100" + +ASN.1 Write mpi 0 (1 limb) +mbedtls_asn1_write_mpi:"00":"020100" ASN.1 Write mpi 1 mbedtls_asn1_write_mpi:"01":"020101" From 2c2730a37274218ffe1e4ddce9d0810ed468d77d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Fri, 10 Jun 2022 20:15:44 +0200 Subject: [PATCH 5/8] ASN.1 write tests: test with larger buffer Test with the output buffer size up to *and including* the expected output size plus one. `... < expected->len + 1` was evidently a mistake. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.function | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index 8d5579d05..16194dcd6 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -63,7 +63,7 @@ void mbedtls_asn1_write_null( data_t *expected ) generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -83,7 +83,7 @@ void mbedtls_asn1_write_bool( int val, data_t *expected ) generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -103,7 +103,7 @@ void mbedtls_asn1_write_int( int val, data_t *expected ) generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -124,7 +124,7 @@ void mbedtls_asn1_write_enum( int val, data_t *expected ) generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -148,7 +148,7 @@ void mbedtls_asn1_write_mpi( data_t *val, data_t *expected ) mbedtls_mpi_init( &mpi ); TEST_ASSERT( mbedtls_mpi_read_binary( &mpi, val->x, val->len ) == 0 ); - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -171,7 +171,7 @@ void mbedtls_asn1_write_string( int tag, data_t *content, data_t *expected ) generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -225,7 +225,7 @@ void mbedtls_asn1_write_algorithm_identifier( data_t *oid, generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; @@ -309,7 +309,7 @@ void test_asn1_write_bitstrings( data_t *bitstring, int bits, ( is_named ? mbedtls_asn1_write_named_bitstring : mbedtls_asn1_write_bitstring ); - for( data.size = 0; data.size < expected->len + 1; data.size++ ) + for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { if( ! generic_write_start_step( &data ) ) goto exit; From b7e215f6bc73bddd9b3c48126682c1547b8948e8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 15 Jun 2022 21:16:42 +0200 Subject: [PATCH 6/8] Fix copypasta in test data Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.data | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/suites/test_suite_asn1write.data b/tests/suites/test_suite_asn1write.data index 6fc668f53..725cbc22a 100644 --- a/tests/suites/test_suite_asn1write.data +++ b/tests/suites/test_suite_asn1write.data @@ -200,7 +200,7 @@ ASN.1 Write OID: length=1 mbedtls_asn1_write_string:MBEDTLS_ASN1_OID:"41":"060141" ASN.1 Write AlgorithmIdentifier, null parameters -mbedtls_asn1_write_algorithm_identifier:"4f4944":8:"300d06034f4944" +mbedtls_asn1_write_algorithm_identifier:"4f4944":0:"300706034f49440500" ASN.1 Write AlgorithmIdentifier, parameters (8 bytes) mbedtls_asn1_write_algorithm_identifier:"4f4944":8:"300d06034f4944" From 6194053feb2a14a42ffaaa8042f39ab025b754f0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Wed, 15 Jun 2022 21:17:25 +0200 Subject: [PATCH 7/8] ASN.1: test that we can parse what we can write In asn1_write tests, when there's a parsing function corresponding to the write function, call it and check that it can parse what we wrote. Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.function | 147 ++++++++++++++++++++- 1 file changed, 144 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index 16194dcd6..8c260f767 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -16,6 +16,8 @@ typedef struct int generic_write_start_step( generic_write_data_t *data ) { mbedtls_test_set_step( data->size ); + mbedtls_free( data->output ); + data->output = NULL; ASSERT_ALLOC( data->output, data->size == 0 ? 1 : data->size ); data->end = data->output + data->size; data->p = data->end; @@ -45,8 +47,6 @@ int generic_write_finish_step( generic_write_data_t *data, ok = 1; exit: - mbedtls_free( data->output ); - data->output = NULL; return( ok ); } @@ -70,6 +70,7 @@ void mbedtls_asn1_write_null( data_t *expected ) ret = mbedtls_asn1_write_null( &data.p, data.start ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; + /* There's no parsing function for NULL. */ } exit: @@ -90,6 +91,14 @@ void mbedtls_asn1_write_bool( int val, data_t *expected ) ret = mbedtls_asn1_write_bool( &data.p, data.start, val ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; +#if defined(MBEDTLS_ASN1_PARSE_C) + if( ret >= 0 ) + { + int read = 0xdeadbeef; + TEST_EQUAL( mbedtls_asn1_get_bool( &data.p, data.end, &read ), 0 ); + TEST_EQUAL( val, read ); + } +#endif /* MBEDTLS_ASN1_PARSE_C */ } exit: @@ -110,6 +119,14 @@ void mbedtls_asn1_write_int( int val, data_t *expected ) ret = mbedtls_asn1_write_int( &data.p, data.start, val ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; +#if defined(MBEDTLS_ASN1_PARSE_C) + if( ret >= 0 ) + { + int read = 0xdeadbeef; + TEST_EQUAL( mbedtls_asn1_get_int( &data.p, data.end, &read ), 0 ); + TEST_EQUAL( val, read ); + } +#endif /* MBEDTLS_ASN1_PARSE_C */ } exit: @@ -131,6 +148,14 @@ void mbedtls_asn1_write_enum( int val, data_t *expected ) ret = mbedtls_asn1_write_enum( &data.p, data.start, val ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; +#if defined(MBEDTLS_ASN1_PARSE_C) + if( ret >= 0 ) + { + int read = 0xdeadbeef; + TEST_EQUAL( mbedtls_asn1_get_enum( &data.p, data.end, &read ), 0 ); + TEST_EQUAL( val, read ); + } +#endif /* MBEDTLS_ASN1_PARSE_C */ } exit: @@ -142,10 +167,11 @@ exit: void mbedtls_asn1_write_mpi( data_t *val, data_t *expected ) { generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; - mbedtls_mpi mpi; + mbedtls_mpi mpi, read; int ret; mbedtls_mpi_init( &mpi ); + mbedtls_mpi_init( &read ); TEST_ASSERT( mbedtls_mpi_read_binary( &mpi, val->x, val->len ) == 0 ); for( data.size = 0; data.size <= expected->len + 1; data.size++ ) @@ -155,12 +181,21 @@ void mbedtls_asn1_write_mpi( data_t *val, data_t *expected ) ret = mbedtls_asn1_write_mpi( &data.p, data.start, &mpi ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; +#if defined(MBEDTLS_ASN1_PARSE_C) + if( ret >= 0 ) + { + TEST_EQUAL( mbedtls_asn1_get_mpi( &data.p, data.end, &read ), 0 ); + TEST_EQUAL( 0, mbedtls_mpi_cmp_mpi( &mpi, &read ) ); + } +#endif /* MBEDTLS_ASN1_PARSE_C */ + /* Skip some intermediate lengths, they're boring. */ if( expected->len > 10 && data.size == 8 ) data.size = expected->len - 2; } exit: mbedtls_mpi_free( &mpi ); + mbedtls_mpi_free( &read ); mbedtls_free( data.output ); } /* END_CASE */ @@ -208,6 +243,8 @@ void mbedtls_asn1_write_string( int tag, data_t *content, data_t *expected ) } if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; + /* There's no parsing function for octet or character strings. */ + /* Skip some intermediate lengths, they're boring. */ if( expected->len > 10 && data.size == 8 ) data.size = expected->len - 2; } @@ -224,6 +261,9 @@ void mbedtls_asn1_write_algorithm_identifier( data_t *oid, { generic_write_data_t data = { NULL, NULL, NULL, NULL, 0 }; int ret; +#if defined(MBEDTLS_ASN1_PARSE_C) + unsigned char *buf_complete = NULL; +#endif /* MBEDTLS_ASN1_PARSE_C */ for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { @@ -240,10 +280,69 @@ void mbedtls_asn1_write_algorithm_identifier( data_t *oid, ret -= par_len; if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; + +#if defined(MBEDTLS_ASN1_PARSE_C) + /* Only do a parse-back test if the parameters aren't too large for + * a small-heap environment. The boundary is somewhat arbitrary. */ + if( ret >= 0 && par_len <= 1234 ) + { + mbedtls_asn1_buf alg = {0, 0, NULL}; + mbedtls_asn1_buf params = {0, 0, NULL}; + /* The writing function doesn't write the parameters unless + * they're null: it only takes their length as input. But the + * parsing function requires the parameters to be present. + * Thus make up parameters. */ + size_t data_len = data.end - data.p; + size_t len_complete = data_len + par_len; + unsigned char expected_params_tag; + size_t expected_params_len; + ASSERT_ALLOC( buf_complete, len_complete ); + unsigned char *end_complete = buf_complete + len_complete; + memcpy( buf_complete, data.p, data_len ); + if( par_len == 0 ) + { + /* mbedtls_asn1_write_algorithm_identifier() wrote a NULL */ + expected_params_tag = 0x05; + expected_params_len = 0; + } + else if( par_len >= 2 && par_len < 2 + 128 ) + { + /* Write an OCTET STRING with a short length encoding */ + expected_params_tag = buf_complete[data_len] = 0x04; + expected_params_len = par_len - 2; + buf_complete[data_len + 1] = (unsigned char) expected_params_len; + } + else if( par_len >= 4 + 128 && par_len < 3 + 256 * 256 ) + { + /* Write an OCTET STRING with a two-byte length encoding */ + expected_params_tag = buf_complete[data_len] = 0x04; + expected_params_len = par_len - 4; + buf_complete[data_len + 1] = 0x82; + buf_complete[data_len + 2] = (unsigned char) ( expected_params_len >> 8 ); + buf_complete[data_len + 3] = (unsigned char) ( expected_params_len ); + } + else + { + TEST_ASSERT( ! "Bad test data: invalid length of ASN.1 element" ); + } + unsigned char *p = buf_complete; + TEST_EQUAL( mbedtls_asn1_get_alg( &p, end_complete, + &alg, ¶ms ), 0 ); + TEST_EQUAL( alg.tag, MBEDTLS_ASN1_OID ); + ASSERT_COMPARE( alg.p, alg.len, oid->x, oid->len ); + TEST_EQUAL( params.tag, expected_params_tag ); + TEST_EQUAL( params.len, expected_params_len ); + mbedtls_free( buf_complete ); + buf_complete = NULL; + } +#endif /* MBEDTLS_ASN1_PARSE_C */ } exit: mbedtls_free( data.output ); +#if defined(MBEDTLS_ASN1_PARSE_C) + mbedtls_free( buf_complete ); +#endif /* MBEDTLS_ASN1_PARSE_C */ } /* END_CASE */ @@ -308,6 +407,34 @@ void test_asn1_write_bitstrings( data_t *bitstring, int bits, const unsigned char *buf, size_t bits ) = ( is_named ? mbedtls_asn1_write_named_bitstring : mbedtls_asn1_write_bitstring ); +#if defined(MBEDTLS_ASN1_PARSE_C) + unsigned char *masked_bitstring = NULL; +#endif /* MBEDTLS_ASN1_PARSE_C */ + + /* The API expects `bitstring->x` to contain `bits` bits. */ + size_t byte_length = ( bits + 7 ) / 8; + TEST_ASSERT( bitstring->len >= byte_length ); + +#if defined(MBEDTLS_ASN1_PARSE_C) + ASSERT_ALLOC( masked_bitstring, byte_length ); + memcpy( masked_bitstring, bitstring->x, byte_length ); + if( bits % 8 != 0 ) + masked_bitstring[byte_length - 1] &= ~( 0xff >> ( bits % 8 ) ); + size_t value_bits = bits; + if( is_named ) + { + /* In a named bit string, all trailing 0 bits are removed. */ + while( byte_length > 0 && masked_bitstring[byte_length - 1] == 0 ) + --byte_length; + value_bits = 8 * byte_length; + if( byte_length > 0 ) + { + unsigned char last_byte = masked_bitstring[byte_length - 1]; + for( unsigned b = 1; b < 0xff && ( last_byte & b ) == 0; b <<= 1 ) + --value_bits; + } + } +#endif /* MBEDTLS_ASN1_PARSE_C */ for( data.size = 0; data.size <= expected->len + 1; data.size++ ) { @@ -316,10 +443,24 @@ void test_asn1_write_bitstrings( data_t *bitstring, int bits, ret = ( *func )( &data.p, data.start, bitstring->x, bits ); if( ! generic_write_finish_step( &data, expected, ret ) ) goto exit; +#if defined(MBEDTLS_ASN1_PARSE_C) + if( ret >= 0 ) + { + mbedtls_asn1_bitstring read = {0, 0, NULL}; + TEST_EQUAL( mbedtls_asn1_get_bitstring( &data.p, data.end, + &read ), 0 ); + ASSERT_COMPARE( read.p, read.len, + masked_bitstring, byte_length ); + TEST_EQUAL( read.unused_bits, 8 * byte_length - value_bits ); + } +#endif /* MBEDTLS_ASN1_PARSE_C */ } exit: mbedtls_free( data.output ); +#if defined(MBEDTLS_ASN1_PARSE_C) + mbedtls_free( masked_bitstring ); +#endif /* MBEDTLS_ASN1_PARSE_C */ } /* END_CASE */ From 5969a4b5e0fb2ab017b05b43d5341a117ce46825 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 27 Jun 2022 23:59:20 +0200 Subject: [PATCH 8/8] Don't call memcpy(NULL, 0) which has undefined behavior Signed-off-by: Gilles Peskine --- tests/suites/test_suite_asn1write.function | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/suites/test_suite_asn1write.function b/tests/suites/test_suite_asn1write.function index 8c260f767..4ed8644f0 100644 --- a/tests/suites/test_suite_asn1write.function +++ b/tests/suites/test_suite_asn1write.function @@ -417,9 +417,12 @@ void test_asn1_write_bitstrings( data_t *bitstring, int bits, #if defined(MBEDTLS_ASN1_PARSE_C) ASSERT_ALLOC( masked_bitstring, byte_length ); - memcpy( masked_bitstring, bitstring->x, byte_length ); - if( bits % 8 != 0 ) - masked_bitstring[byte_length - 1] &= ~( 0xff >> ( bits % 8 ) ); + if( byte_length != 0 ) + { + memcpy( masked_bitstring, bitstring->x, byte_length ); + if( bits % 8 != 0 ) + masked_bitstring[byte_length - 1] &= ~( 0xff >> ( bits % 8 ) ); + } size_t value_bits = bits; if( is_named ) {