mbedtls PKCS7 Parser

Hi,

This pull request is to add PKCS7 parser feature in mbedtls. This is following RFC 2315

The support is limited to parsing of SignedData content type. This is the base PKCS7 parser work done based on our current requirement and can easily be extended to full fledged parser over the time as needed. Currently, it doesn't handle CRLs which are optional in PKCS7 spec and looks for single signer info.

I had to do an additional fix to make the build work using CMake in gcc9.0. I had to disable -Wtype-limits error by adding -Wno-type-limits. This is a general patch to fix the error reported in SSL. It doesn't have anything to do with PKCS7, but is identified while testing pkcs7 build with cmake.

Status

READY

Migrations

There is no API change here. Its a new feature added.

Additional comments

Since the files - library/error.c and library/version_features.c and programs/test/query_config.c got generated by running the scripts, I have added them as separate commits.

There are few additional commits added separately which are on top of base PKCS7 work.

Steps to test or reproduce

Test suite is written which checks for both failure/success cases. They will get run as part of "make test" "cmake test" "cd tests && ./test_suite_pkcs7".
One can try to write a program to parse the pkcs7 based signature using: mbedtls_pkcs7_parse_der() and then mbedtls_pkcs7_signed_data_verify() function.

Jun 15 '20 16:06 naynajain

Thank you for your contribution! I've made a first, superficial pass over it. Before we do an in-depth review:

Please address the issues I raised (separating bug fixes; ).
Please add a changelog entry file under ChangeLog.d for the new feature.
Please address whatever's causing the travis-ci job to fail. Once this job passes, if there are failures in the jenkins jobs, which run on a private server, ping us to let you know what's failing.

I have not done an architectural review. I don't expect surprises here, since this is adding a module with a well-defined purpose.

Unfortunately, our bandwidth for reviews is full at the moment, so it may be a while before we can review and merge this code. Before contributing a major feature, it's a good idea to mention it on the mailing list first, to discuss the architecture and set expectations for the schedule.

Jun 15 '20 20:06 gilles-peskine-arm

Thank you for your contribution! I've made a first, superficial pass over it. Before we do an in-depth review:
* Please address the issues I raised (separating bug fixes; ).

* Please add a changelog entry file under `ChangeLog.d` for the new feature.

* Please address whatever's causing the travis-ci job to fail. Once this job passes, if there are failures in the jenkins jobs, which run on a private server, ping us to let you know what's failing.
I have not done an architectural review. I don't expect surprises here, since this is adding a module with a well-defined purpose.

Unfortunately, our bandwidth for reviews is full at the moment, so it may be a while before we can review and merge this code. Before contributing a major feature, it's a good idea to mention it on the mailing list first, to discuss the architecture and set expectations for the schedule.

Thanks for your feedback and quick response.

I will include your feedback and create two PRs. One PR for PKCS7 parser feature and another for bug fix.

The bug fix PR will have following two fixes: 69252ab - mbedtls: Disables type-limit error. (After fixing the issue) 287d6d0 - mbedtls: x509: do not include time.h without MBEDTLS_HAVE_TIME

As per mailing list discussion, do you mean to post the patches in the mailing-list or a proposal mail for new feature referring this PR ?

Thanks & Regards,
- Nayna

Jun 16 '20 22:06 naynajain

As per mailing list discussion, do you mean to post the patches in the mailing-list or a proposal mail for new feature referring this PR ?

I think Gilles meant in future it's better to discuss big features on the mailing list before creating a PR to avoid wasting your time. Now you've already created the PR, that's not necessary but we might not have the bandwidth to review this properly for a while.

Hopefully the bug fix PR should be easier to absorb quickly.

Jul 02 '20 16:07 danh-arm

Hi @naynajain,

Thank you very much for your contribution, you did a great job for your first PR on Mbed TLS!

Unfortunately, there are numerous pitfalls as well as corresponding test requirements when writing ASN.1 parsing code, which makes any work in this area quite tedious and error prone. Specifically, I noticed that your parsing code doesn't consistently obey the nested bounds of the structures that are being parsed. For example, when you enter an ASN.1 SEQUENCE, the end-of-buffer pointer should be set to the end of that SEQUENCE while parsing the entries of the SEQUENCE. The current code doesn't do this in numerous places, and needs to be reworked accordingly.

Ideally, we should have negative unit tests test_suite_pkcs7 which cover all those edge cases. This is a pain to write (see e.g. https://github.com/ARMmbed/mbedtls/blob/development/tests/suites/test_suite_x509parse.data#L1014), but ultimately is the best way to ensure that we got the code right. Those negative tests should ideally be written in a way that the presented ASN.1 data is flawed in precisely one place, while trying to maintain structural sanity everywhere else. For example, if you have a structure which contains a SEQUENCE as a field, then ideally a negative test which triggers an error in this inner SEQUENCE parsing should still complete the outer structure correctly. This gives the implementation the freedom to switch e.g. between depth-first and breadth-first parsing, without requiring all tests to be rewritten.

Would you be willing to adjust the code, and to start writing some of the required negative tests?

Thank you so much for reviewing and sharing your feedbacks.

Yes, sure I would include both code changes and negative tests.

Thanks & Regards, - Nayna

Aug 14 '20 15:08 naynajain

I have just now pushed the patches that includes the earlier feedbacks. The changes include:

This set strictly supports single signer and correspondingly single certificate and no CRL. Accordingly, the bounds checks are added.
I have removed the static test data and is now generating the data via commands in tests/data_files/Makefile. I have also added negative test cases. These may not be very extensive in the list but did a start and am hoping to improve it over the time. I hope they are as expected.
One of the major change is the second patch which allows parsing of the files which are just "Signed Data"
I have changed the range for errors as the previous one was already in use.
The two bugfixes patches, which were handled separately, are removed.

Looking forward for the review and feedback.

Thanks & Regards, - Nayna

Nov 03 '20 22:11 naynajain

Thanks for the rework! However this pull request is not yet ready for review.

All commits must have a valid signoff line, otherwise we are not allowed to take them. The DCO check must pass.
Please rebase on top of a recent version of development so that there are no merge conflicts.
Don't use Unix headers such as unistd.h. (Explains the Windows and bare-metal builds failing on CI.)
test_suite_pkcs7 tries to use X.509 functions without an #if for the appropriate MBEDTLS_X509_xxx. (Explains build_crypto_* failing on CI.)
test_suite_pkcs7 fails in most if not all configurations.
tests/scripts/check_files.py fails.
tests/scripts/doxygen.sh fails. Note that our reference version of Doxygen is 1.8.11 (the one on Ubuntu 16.04) and recent versions are known to have warnings.
Possibly other CI failures.

Nov 04 '20 09:11 gilles-peskine-arm

Thanks for the rework! However this pull request is not yet ready for review.
1. All commits must have a valid signoff line, otherwise we are not allowed to take them. The DCO check must pass.

I rechecked this. All the commits did have Signed-off-by, unless I am really missing something. Lets see I hope this is not the issue now.

2. Please rebase on top of a recent version of `development` so that there are no merge conflicts.

Sure.

3. Don't use Unix headers such as `unistd.h`. (Explains the Windows and bare-metal builds failing on CI.)

Aah.. yes.. ok. Doing it now.

4. `test_suite_pkcs7` tries to use X.509 functions without an `#if` for the appropriate `MBEDTLS_X509_xxx`. (Explains `build_crypto_*` failing on CI.)

Should it be #ifdef or DEPENDENCIES. I think I need to understand this behavior better. As of now I added it as DEPENDENCIES.

5. `test_suite_pkcs7` fails in most if not all configurations.

Is there a way by which I can verify this ? Also, I am not able to access the CI links.. How do I look at all the errors ? I am pushing again now.

6. `tests/scripts/check_files.py` fails.

Sorry, yes I missed to run it last time. I did it now.

7. `tests/scripts/doxygen.sh` fails. Note that our reference version of Doxygen is 1.8.11 (the one on Ubuntu 16.04) and recent versions are known to have warnings.

Again, sorry I missed this also. I did it now.

8. Possibly other CI failures.

Again, how do I check this to know what is failing.

I am pushing the changes again, based on this feedback. Hope it atleast passes DCO checks. I think I need to understand different configuration testing and CI part better. It would be helpful if I can see some logs or details on errors.

Thanks & Regards, - Nayna

Nov 17 '20 15:11 naynajain

DCO is passed now. Seems like it need to have Signed-off-by also of the submitter. I added my Signed-off as well along with Daniel's in the commit "pkcs7: provide fuzz harness ".

Thanks & Regards, - Nayna

Nov 17 '20 15:11 naynajain

I looked at the CI results. I think the pkcs7 tests are failing because it is not able to find the test data files. Now there are no static files, they should be generated via data_files/Makefile. I thought that Makefile would get run. Isn't that the case ? Should I also upload static test data files ? Can you please help to understand this part ?

Nov 18 '20 01:11 naynajain

Hi Nayna,

Looking at tests/data_files/Makefile, I see:

## Build the generated test data. Note that since the final outputs
## are committed to the repository, this target should do nothing on a
## fresh checkout. Furthermore, since the generation is randomized,
## re-running the same targets may result in differing files. The goal
## of this makefile is primarily to serve as a record of how the
## targets were generated in the first place.
default: all_final

So I think you need to run make in that directory to create the test files and then commit them. I've done this in my local tree where I'm working on multiple signer support.

Nov 18 '20 12:11 daxtens

Hi Nayna,

Looking at tests/data_files/Makefile, I see:
## Build the generated test data. Note that since the final outputs
## are committed to the repository, this target should do nothing on a
## fresh checkout. Furthermore, since the generation is randomized,
## re-running the same targets may result in differing files. The goal
## of this makefile is primarily to serve as a record of how the
## targets were generated in the first place.
default: all_final
So I think you need to run make in that directory to create the test files and then commit them. I've done this in my local tree where I'm working on multiple signer support.

Thanks Daniel !! I think I misinterpreted the earlier review comment related to "adding commands in tests/data_files/Makefile... " . I assumed that data will be generated from it as it also saves pushing static files, and so I also removed all the static data which I had in first PR.. I have pushed another patch now for test data. Hopefully CI will pass now.

Nov 18 '20 15:11 naynajain

Aah.. I looked at the feedback now. Thanks for review. Working on them..

Nov 19 '20 17:11 naynajain

I have done all the changes as being suggested except dependency on PEM. I do not call any PEM specific function directly, it is handled via x509_crt code. And I have already added dependency on X509_CRT_PARSE_C. For that reason, I am not sure if PEM dependency is really needed.

As per others.. I have ensured:

Renaming of text files so that check_files.py passes
Fixing to make sure memory_buffer_alloc tests pass.
Fixed other dependencies by checking via ./tests/scripts/depends-{hashes/pkalgs}.pl

I am having error running - tests/scripts/all.sh -k test_no_pem_no_fs, as it looks for this i686-w64-mingw32-gcc. I am having some issues setting it up easily. However, I did look at other test functions to look for PEM dependency, and couldn't see if we really need it for PKCS7.

Thanks & Regards, - Nayna

Nov 25 '20 15:11 naynajain

We have pushed the changes again, seems Travis-CI is passing now. I see continuous-integration/jenkins/pr-head and PR-3431-head TLS Testing is showing as Failed.

When I click on Details, it just gives the error "Server Not Found" . So not sure what is the issue..

@gilles-peskine-arm , can you please share the details, or suggest what is failing.

Thanks & Regards, - Nayna

Dec 15 '20 20:12 naynajain

Hi @naynajain ! Regarding the CI failures, unfortunately the results are no public yet except for Travis, so the error you're getting is fully expected. We have plans for a fully public CI, but it probably won't happen before mid-2021, so in the meantime you need to ask a member of the team to have a look and copy-paste the relevant info - just like you did. Since Gilles is away for a couple of week, I'll have a look this time.

The windows 2013 part is failing with:

         65/86 Test #65: pkcs7-suite ............................***Failed    0.01 sec
         PKCS7 Signed Data Parse Pass SHA256 #1 ............................ PASS
         PKCS7 Signed Data Parse Pass SHA1 #2 .............................. PASS
         PKCS7 Signed Data Parse Pass Without CERT #3 ...................... PASS
         PKCS7 Signed Data Parse Fail with multiple signers #4 ............. PASS
         PKCS7 Signed Data Parse Fail with multiple certs #4 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted cert #5 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted signer info #6 ........ PASS
         PKCS7 Signed Data Parse Fail Version other than 1 #7 .............. PASS
         PKCS7 Signed Data Parse Fail Encrypted Content #8 ................. PASS
         PKCS7 Signed Data Verification Pass SHA256 #9 ..................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA256 #9.1 ................... FAILED
           buflen == datalen
           at line 279, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA1 #10 ...................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA512 #11 .................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different certifica  FAILED
           buflen == datalen
           at line 336, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different data hash  FAILED
           buflen == datalen
           at line 386, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.issuer #15.1 ... PASS
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.serial #15.2 ... PASS
         PKCS7 Only Signed Data Parse Pass #15 ............................. PASS
         
         ----------------------------------------------------------------------------
         
         FAILED (12 / 18 tests (0 skipped))

The windows-mingw part is also failing in test_suite_pkcs7.

I'm not familiar with the contents of the PR, so I won't hazard a guess as to why this test would fail on windows while passing on Linux.

The all.sh test_full_cmake_gcc_asan part failed to clone the repo, so this failure's unrelated to your PR.

The all.sh test_no_pem_no_fs part failed to build:

CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_without_cert':
test_suite_pkcs7.c:(.text+0x93e): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_version':
test_suite_pkcs7.c:(.text+0xb9e): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_content_oid':
test_suite_pkcs7.c:(.text+0xdfe): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_failure':
test_suite_pkcs7.c:(.text+0x10ae): undefined reference to `mbedtls_pkcs7_load_file'
collect2: error: ld returned 1 exit status

I would guess this is probably about some missing dependencies in a test functions. you should be able to reproduce this one locally (on a Unix-y machine) by running tests/scripts/all.sh test_no_pem_no_fs.

Finally, the all.sh test_no_platform part failed with:

suites/test_suite_pkcs7.function: In function 'test_pkcs7_verify':
suites/test_suite_pkcs7.function:206:11: error: implicit declaration of function 'mbedtls_x509_crt_parse_file' [-Werror=implicit-function-declaration]
     res = mbedtls_x509_crt_parse_file( &x509, crt );
           ^

I would also suspect a dependency issue here. Can be reproduced by running tests/scripts/all.sh test_no_platform as well.

Hope this helps, and if there are any more failures with the next iteration of your PR, don't hesitate to ping us for more details on the issues.

Dec 18 '20 09:12 mpg

Hi @naynajain ! Regarding the CI failures, unfortunately the results are no public yet except for Travis, so the error you're getting is fully expected. We have plans for a fully public CI, but it probably won't happen before mid-2021, so in the meantime you need to ask a member of the team to have a look and copy-paste the relevant info - just like you did. Since Gilles is away for a couple of week, I'll have a look this time.

The windows 2013 part is failing with:

         65/86 Test #65: pkcs7-suite ............................***Failed    0.01 sec
         PKCS7 Signed Data Parse Pass SHA256 #1 ............................ PASS
         PKCS7 Signed Data Parse Pass SHA1 #2 .............................. PASS
         PKCS7 Signed Data Parse Pass Without CERT #3 ...................... PASS
         PKCS7 Signed Data Parse Fail with multiple signers #4 ............. PASS
         PKCS7 Signed Data Parse Fail with multiple certs #4 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted cert #5 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted signer info #6 ........ PASS
         PKCS7 Signed Data Parse Fail Version other than 1 #7 .............. PASS
         PKCS7 Signed Data Parse Fail Encrypted Content #8 ................. PASS
         PKCS7 Signed Data Verification Pass SHA256 #9 ..................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA256 #9.1 ................... FAILED
           buflen == datalen
           at line 279, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA1 #10 ...................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA512 #11 .................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different certifica  FAILED
           buflen == datalen
           at line 336, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different data hash  FAILED
           buflen == datalen
           at line 386, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmpl1zzjj0i/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.issuer #15.1 ... PASS
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.serial #15.2 ... PASS
         PKCS7 Only Signed Data Parse Pass #15 ............................. PASS
         
         ----------------------------------------------------------------------------
         
         FAILED (12 / 18 tests (0 skipped))

The windows-mingw part is also failing in test_suite_pkcs7.

I'm not familiar with the contents of the PR, so I won't hazard a guess as to why this test would fail on windows while passing on Linux.

The all.sh test_full_cmake_gcc_asan part failed to clone the repo, so this failure's unrelated to your PR.

The all.sh test_no_pem_no_fs part failed to build:

CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_without_cert':
test_suite_pkcs7.c:(.text+0x93e): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_version':
test_suite_pkcs7.c:(.text+0xb9e): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_content_oid':
test_suite_pkcs7.c:(.text+0xdfe): undefined reference to `mbedtls_pkcs7_load_file'
CMakeFiles/test_suite_pkcs7.dir/test_suite_pkcs7.c.o: In function `test_pkcs7_parse_failure':
test_suite_pkcs7.c:(.text+0x10ae): undefined reference to `mbedtls_pkcs7_load_file'
collect2: error: ld returned 1 exit status

I would guess this is probably about some missing dependencies in a test functions. you should be able to reproduce this one locally (on a Unix-y machine) by running tests/scripts/all.sh test_no_pem_no_fs.

Finally, the all.sh test_no_platform part failed with:

suites/test_suite_pkcs7.function: In function 'test_pkcs7_verify':
suites/test_suite_pkcs7.function:206:11: error: implicit declaration of function 'mbedtls_x509_crt_parse_file' [-Werror=implicit-function-declaration]
     res = mbedtls_x509_crt_parse_file( &x509, crt );
           ^

I would also suspect a dependency issue here. Can be reproduced by running tests/scripts/all.sh test_no_platform as well.

Hope this helps, and if there are any more failures with the next iteration of your PR, don't hesitate to ping us for more details on the issues.

Thanks Manuel for the detailed report on errors.

Nick had today pushed the patch that should fix failures because of tests/scripts/all.sh test_no_pem_no_fs We also verified tests/scripts/all.sh test_no_platform, and it seems to be passing in our local setup.

Can you please look at the Travis logs again and check the reasons for the failure ? I wonder if it is because of - all.sh test_full_cmake_gcc_asan which is unrelated to our code.

Thanks & Regards, - Nayna

Jan 05 '21 20:01 naynajain

Thanks for fixing those failures! The remaining failures seem to only happen on windows and to be related to this PR, as test_suite_pkcs7 is failing with:

         PKCS7 Signed Data Parse Pass SHA256 #1 ............................ PASS
         PKCS7 Signed Data Parse Pass SHA1 #2 .............................. PASS
         PKCS7 Signed Data Parse Pass Without CERT #3 ...................... PASS
         PKCS7 Signed Data Parse Fail with multiple signers #4 ............. PASS
         PKCS7 Signed Data Parse Fail with multiple certs #4 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted cert #5 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted signer info #6 ........ PASS
         PKCS7 Signed Data Parse Fail Version other than 1 #7 .............. PASS
         PKCS7 Signed Data Parse Fail Encrypted Content #8 ................. PASS
         PKCS7 Signed Data Verification Pass SHA256 #9 ..................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA256 #9.1 ................... FAILED
           buflen == datalen
           at line 279, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA1 #10 ...................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA512 #11 .................... FAILED
           buflen == datalen
           at line 225, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different certifica  FAILED
           buflen == datalen
           at line 336, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different data hash  FAILED
           buflen == datalen
           at line 386, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp6he90804/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.issuer #15.1 ... PASS
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.serial #15.2 ... PASS
         PKCS7 Only Signed Data Parse Pass #15 ............................. PASS
         
         ----------------------------------------------------------------------------
         
         FAILED (12 / 18 tests (0 skipped))

On our Jenkins instance, this is happening on Windows when compiling with cmake and MSVC 2013. It also happens when compiling with MinGW, but for MinGW I don't have details of which tests are failing.

This might be due to some C types having a different size on Windows - just a random guess.

Jan 06 '21 09:01 mpg

Hi,

I think the failure on Windows was because of difference in fread() behavious between Windows and Linux.

I did a change in ASSERT test to consider the difference, but I didn't have a way to verify on a Windows system and had just pushed the change. It seems there is still the failure. Also, there are two other links showing failure pr-merge.. Can you please share again the details ?

Sorry for the repetitive trouble.

Thanks & Regards, - Nayna

Jan 07 '21 02:01 naynajain

Thanks for fixing those failures. The remaining failures now happen later, so we seem to be making progress:

         65/86 Test #65: pkcs7-suite ............................***Failed    0.02 sec
         PKCS7 Signed Data Parse Pass SHA256 #1 ............................ PASS
         PKCS7 Signed Data Parse Pass SHA1 #2 .............................. PASS
         PKCS7 Signed Data Parse Pass Without CERT #3 ...................... PASS
         PKCS7 Signed Data Parse Fail with multiple signers #4 ............. PASS
         PKCS7 Signed Data Parse Fail with multiple certs #4 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted cert #5 ............... PASS
         PKCS7 Signed Data Parse Fail with corrupted signer info #6 ........ PASS
         PKCS7 Signed Data Parse Fail Version other than 1 #7 .............. PASS
         PKCS7 Signed Data Parse Fail Encrypted Content #8 ................. PASS
         PKCS7 Signed Data Verification Pass SHA256 #9 ..................... FAILED
           res == 0
           at line 230, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp97iwiqxk/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA256 #9.1 ................... FAILED
           res == 0
           at line 291, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp97iwiqxk/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA1 #10 ...................... FAILED
           res == 0
           at line 230, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp97iwiqxk/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Pass SHA512 #11 .................... FAILED
           res == 0
           at line 230, C:/builds/workspace/mbed-tls-pr-head_PR-3431-head/worktrees/tmp97iwiqxk/tests/suites/test_suite_pkcs7.function
         PKCS7 Signed Data Verification Fail because of different certifica  PASS
         PKCS7 Signed Data Verification Fail because of different data hash  PASS
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.issuer #15.1 ... PASS
         PKCS7 Signed Data Parse Failure Corrupt signerInfo.serial #15.2 ... PASS
         PKCS7 Only Signed Data Parse Pass #15 ............................. PASS
         
         ----------------------------------------------------------------------------
         
         FAILED (14 / 18 tests (0 skipped))

Sorry for the repetitive trouble.

Don't worry, I don't have access to a Windows machine either, so I know how painful it can be to debug this kind of issues.

Jan 07 '21 09:01 mpg

I had a quick look at your last commit and I'm not convinced by this change. According to Microsoft documentation (which tends to be correct), fread() has the same return convention on windows as on Unix, so I think the == assert was correct. Perhaps the right way to fix would be to open in binary mode, as the commit message suggests.

We have a helper function for reading files in the PK module that does exactly that: open in binary mode, then assert the the return value is == to the the expected number of bytes, and seems to be working well enough on all platforms tested so far. https://github.com/ARMmbed/mbedtls/blob/development/library/pkparse.c#L73 You could probably either use this function directly (after all, PKCS7 already depends on PK unless I'm mistaken) or (if it does things you don't want, like appending the 0 byte and including it in the length if the file looks like PEM) just use it as a working example.

Oh, now that I look a bit more closely to your code, I see you actually have a similar function mbedtls_pkcs7_load_file(). It's not immediately clear to me then why in the test function you sometimes use mbedtls_pkcs7_load_file() and sometimes directly fopen() & Co.

Also, I think perhaps using stat() to get the size of the file might actually be the source of the problem - I'm not very familiar with this but perhaps there's a reason our helper function used fseek() instead.

Jan 07 '21 09:01 mpg

Hi @naynajain ! I wanted to update you about two things regarding this PR:

I'll be on leave from Monday until the 27th of January, so it you need access to CI results, please ping another team member. Sorry we don't have a public CI yet, I realise this is tedious. (We have plans for a public CI but it won't happen before the middle of the year.)
Since this PR was created, we refined our process for handling community contributions: small enough PRs are still reviewed as they come, but for larger PRs we realise this didn't work well, so instead now we're putting them in our backlog and scheduling them as we do for our own development work - that way when we schedule the PR, we know we have two active reviewers with enough bandwidth. It also sets more realistic expectations for contributors (for example, over the 6 months since the creation of this PR, activity on our side has been intermittent and not very predictable.) All this to say that your PR falls on the larger end of the spectrum (and thanks for making such a big contribution, it's really appreciated!), so I'm moving it to our backlog, where it will wait until we can schedule it, at which point we'll inform you and hopefully have an efficient review-rework loop until the PR gets merged.

Jan 08 '21 10:01 mpg

I had a quick look at your last commit and I'm not convinced by this change. According to Microsoft documentation (which tends to be correct), fread() has the same return convention on windows as on Unix, so I think the == assert was correct. Perhaps the right way to fix would be to open in binary mode, as the commit message suggests.

I did already try "binary mode" before adding this ASSERT check, and that time also test failed. But I didn't verify against results and had just assumed the failure as "binary mode" not working.

We have a helper function for reading files in the PK module that does exactly that: open in binary mode, then assert the the return value is == to the the expected number of bytes, and seems to be working well enough on all platforms tested so far. https://github.com/ARMmbed/mbedtls/blob/development/library/pkparse.c#L73 You could probably either use this function directly (after all, PKCS7 already depends on PK unless I'm mistaken) or (if it does things you don't want, like appending the 0 byte and including it in the length if the file looks like PEM) just use it as a working example.

Oh, now that I look a bit more closely to your code, I see you actually have a similar function mbedtls_pkcs7_load_file(). It's not immediately clear to me then why in the test function you sometimes use mbedtls_pkcs7_load_file() and sometimes directly fopen() & Co.

The pkcs7 tests here is to verify a signature on the data file using public key. mbedtls_pkcs7_load_file() is used to read PKCS7 DER format. fopen() is used to read the data file that just contains some text and is being signed.

Also, I think perhaps using stat() to get the size of the file might actually be the source of the problem - I'm not very familiar with this but perhaps there's a reason our helper function used fseek() instead.

I will check this.

Thanks & Regards, - Nayna

Jan 11 '21 18:01 naynajain

Hi @naynajain ! I wanted to update you about two things regarding this PR:

1. I'll be on leave from Monday until the 27th of January, so it you need access to CI results, please ping another team member. Sorry we don't have a public CI yet, I realise this is tedious. (We have plans for a public CI but it won't happen before the middle of the year.)

Thanks Manuel for keeping me informed. Thanks for all the help.

2. Since this PR was created, we refined our process for handling community contributions: small enough PRs are still reviewed as they come, but for larger PRs we realise this didn't work well, so instead now we're putting them in our backlog and scheduling them as we do for our own development work - that way when we schedule the PR, we know we have two active reviewers with enough bandwidth. It also sets more realistic expectations for contributors (for example, over the 6 months since the creation of this PR, activity on our side has been intermittent and not very predictable.) All this to say that your PR falls on the larger end of the spectrum (and thanks for making such a big contribution, it's really appreciated!), so I'm moving it to our backlog, where it will wait until we can schedule it, at which point we'll inform you and hopefully have an efficient review-rework loop until the PR gets merged.

Thanks Manuel. Would you be able to give us also the heads up for timing when you plan to schedule this PR ? This will help us also to plan our time.

Thanks & Regards, - Nayna

Jan 11 '21 18:01 naynajain

Hello,

We have been trying to work on recreating the failed CI tests. Since the Jenkins tests are internal, we have been using the comments left by Manuel here. On a Windows machine, I built with cmake and Visual Studio 2013. I then ran the relevant test suites that were failing and saw that they all passed. We were hoping to get some more information about how to recreate the issue. My one concern is that we are using a later version of MSVC 2013, MSVC Full Version 180040629. Apparently, there are 5 earlier versions of MSVC 2013. So maybe the CI tests are using an earlier version?

Here is the build process I have been taking:

I use cmake with MSVC 2013 compiler and testing enabled
I then build with cmake --build
I run the tests that were failing

Any additional information on how we can recreate the failures would be greatly appreciated.

Thanks, Nick

Jan 28 '21 15:01 nick-child-ibm

Ouch, tests that only fail on the CI but pass locally are no fun. Here's an excerpt from the CI log with information about the versions and command used:

2021-01-06 17:55:09,464 - VS2013 Release Win32 shipped - INFO - Building mbed TLS using Visual Studio v2013
2021-01-06 17:55:09,464 - VS2013 Release Win32 shipped - INFO - retarget=v120
2021-01-06 17:56:18,030 - VS2013 Release Win32 shipped - INFO - Microsoft Windows [Version 10.0.14393]
(c) 2016 Microsoft Corporation. All rights reserved.

C:\builds\workspace\mbed-tls-pr-head_PR-3431-head\worktrees\tmpqxo2zhaf\visualc\VS2010>"C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat" x86

C:\builds\workspace\mbed-tls-pr-head_PR-3431-head\worktrees\tmpqxo2zhaf\visualc\VS2010>msbuild /nodeReuse:false /t:Rebuild /p:Configuration=Release,Platform=Win32,PlatformToolset=v120 /m "mbedTLS.sln"
Microsoft (R) Build Engine version 12.0.40629.0
[Microsoft .NET Framework, version 4.0.30319.42000]

I'm not sure what else might be relevant here.

Feb 01 '21 09:02 mpg

The pkcs7 tests here is to verify a signature on the data file using public key. mbedtls_pkcs7_load_file() is used to read PKCS7 DER format. fopen() is used to read the data file that just contains some text and is being signed.

I understand, but since there is nothing in mbedtls_pkcs7_load_file() that's really specific to PKCS7 DER format (it just read a bunch of bytes and adds a terminating 0, which by the way I'm not sure it should do, that's only useful for PEM-formated files) I think one could still reuse that. Alternatively, to keep things separate, one might define a static load_file() function in tests/suites/test_suite_pkcs7.c (in the HEADER section) that uses the same strategy: fopen - fseek - fseek - calloc - fread - fclose.

I'm not sure what's going wrong with the current test code, maybe it's stat(), maybe something else, but what I'm saying is this fseek-based strategy always worked so far, so re-using it is probably the simplest thing to do.

Feb 01 '21 09:02 mpg

Ouch, tests that only fail on the CI but pass locally are no fun. Here's an excerpt from the CI log with information about the versions and command used:

2021-01-06 17:55:09,464 - VS2013 Release Win32 shipped - INFO - Building mbed TLS using Visual Studio v2013
2021-01-06 17:55:09,464 - VS2013 Release Win32 shipped - INFO - retarget=v120
2021-01-06 17:56:18,030 - VS2013 Release Win32 shipped - INFO - Microsoft Windows [Version 10.0.14393]
(c) 2016 Microsoft Corporation. All rights reserved.

C:\builds\workspace\mbed-tls-pr-head_PR-3431-head\worktrees\tmpqxo2zhaf\visualc\VS2010>"C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat" x86

C:\builds\workspace\mbed-tls-pr-head_PR-3431-head\worktrees\tmpqxo2zhaf\visualc\VS2010>msbuild /nodeReuse:false /t:Rebuild /p:Configuration=Release,Platform=Win32,PlatformToolset=v120 /m "mbedTLS.sln"
Microsoft (R) Build Engine version 12.0.40629.0
[Microsoft .NET Framework, version 4.0.30319.42000]

I'm not sure what else might be relevant here.

Welcome back Manuel, hope you had a nice and refreshing break. I ran the commands above and everything built fine. How do I run the tests now? I have been running everything with cmake so I am not sure what the equivalent of "ENABLE_TESTING=On" and executing tests are for direct visual studio commands.

Thanks, Nick

Feb 01 '21 16:02 nick-child-ibm

Hi Nick,

Welcome back Manuel, hope you had a nice and refreshing break.

Yes, it was very refreshing indeed :)

I ran the commands above and everything built fine. How do I run the tests now? I have been running everything with cmake so I am not sure what the equivalent of "ENABLE_TESTING=On" and executing tests are for direct visual studio commands.

Oh, my mistake, I quoted the wrong portion from the log. Taking a closer look at our windows testing script, I see that we perform two kinds of builds: one using the .sln file that's distributed with Mbed TLS, but where we only run the selftest program not the test suites, and the other kind of build is done using cmake and that's the one that runs the test suites. Sorry for the confusion.

The cmake commands used are generated by a python script which is unfortunately not public at the moment and are not included in the log, and unfortunately working out the exact commands run would require more time than I have for this these days. However, it looks like it's very similar to what you did. The only major difference is the Mbed TLS configuration used, we start with scripts/config.py full then disable the following: options MBEDTLS_MEMORY_DEBUG, MBEDTLS_MEMORY_BACKTRACE, MBEDTLS_MEMORY_BUFFER_ALLOC_C, MBEDTLS_THREADING_PTHREAD, MBEDTLS_THREADING_ALT, MBEDTLS_THREADING_C, MBEDTLS_DEPRECATED_WARNING. However I don't think this would explain the particular failure we're facing.

I'm running out of time to spend on this PR for now, and I'm far from being a windows expert, but I'd like to suggest again rewriting the file reading part of the test function to use the same fseek()-based idiom as mbedtls_pkcs7_load_file() - either by calling this function directly or creating a test-specific helper function, which might just solve the problem. Granted, if the problem wss solved that way, it would be less satisfying than first being able to reproduce it and fully understand what's going wrong, but still, if this approach has a chance of solving the problem it's probably worth trying.

Feb 02 '21 12:02 mpg

Thanks Manuel. Would you be able to give us also the heads up for timing when you plan to schedule this PR ? This will help us also to plan our time.

That's a very reasonable request. Unfortunately right now I don't have the answer, but I can attempt an educated guess.

The list of large community PRs waiting to be scheduled is publicly available on our backlog board: https://github.com/orgs/ARMmbed/projects/6?card_filter_query=label%3Acommunity+is%3Apr - as you can see there are currently three PRs in addition to the two PKCS7 PRs. The two PRS about EdDSA and Raw public keys are in high demand and have been requested for several years now, so they're likely to be scheduled first. Also, during the first half of the year, our main focus will be preparing Mbed TLS 3.0, so we'll have less resources than usual for other things.

Considering this, I'm afraid we might not be able to schedule time to review this pair of PRs until the second half of 2021. I realise this is not very satisfying. Since Mbed TLS became an open governance project at the beginning of 2020, we've considerably increased the resources we devote to working with the community; unfortunately over the preceding years we had accumulated a significant backlog that we're still working through (the first raw public keys PR was submitted 2015 for example and we still have to schedule it) - while this is in progress, unfortunately the latency on reviewing large contributions is still higher than we'd like.

We have quarterly planning meetings where we review our roadmap, and then monthly meetings for scheduling work; I'll be sure to keep you posted when we have more information about when we might be looking at this pair of PRs. In the meantime, thank you for your patience!

Feb 02 '21 12:02 mpg

We have quarterly planning meetings where we review our roadmap, and then monthly meetings for scheduling work; I'll be sure to keep you posted when we have more information about when we might be looking at this pair of PRs. In the meantime, thank you for your patience!

Hi Manuel, I hope you had great vacation. I really appreciate the detailed update from you.

I am thinking in the meantime atleast we should try to get the CI through. We would need help from you on sharing the error till we get it resolved. Does that sound ok ?

I did try your suggestion to use file_load(). I used the one defined in from PKCS7 code. It again worked for me and for Nick on his local Windows setup, but failed on CI. Can you please share the error details again ?

Feb 10 '21 23:02 naynajain

mbedtls
mbedtls copied to clipboard

PKCS7 Parser - RFC 2315

Status

Migrations

Additional comments

Steps to test or reproduce

mbedtls mbedtls copied to clipboard

PKCS7 Parser - RFC 2315

Status

Migrations

Additional comments

Steps to test or reproduce

mbedtls
mbedtls copied to clipboard