gh-ARMmbed/mbed-crypto
Watch
1
0
Fork
You've already forked mbed-crypto
 0
Code Issues 36 Pull requests 1 Releases 7 Activity

ASN.1 tests without x509 #75

Merged
gilles-peskine-arm merged 17 commits from asn1-tests-without-x509 into development 2019-10-04 06:30:02 -05:00
Conversation 11 Commits 17 Files changed 13 +1833 -211
gilles-peskine-arm commented 2019-03-01 16:29:37 -06:00 (Migrated from github.com)

Add ASN.1 unit tests. Cover all functions from the asn1 parse and write modules. Fix #76.

Fix a couple of minor bugs and improve the documentation.

This PR breaks mbedtls because two test cases in test_suite_x509_parse make assumptions about bugs in the ASN.1 module which this PR fixes. This PR must land in mbedtls only once https://github.com/ARMmbed/mbedtls/pull/2488 is merged there.

Once this PR is merged, a commented-out test case that https://github.com/ARMmbed/mbedtls/pull/2488 adds to test_suite_x509_parse.data can be uncommented.

Backports:

  • ASN.1 parsing bug fixes: I don't think so. The bugs I fixed are minor. They cause valid but uncommon data to be rejected. If it hasn't bothered anyone in all that time, it's best not to take any risks.
  • New tests: no. In earlier versions, ASN.1 was tested indirectly through X.509. The ASN.1 tests improve the coverage, but I don't think it's worth backporting them
  • Documentation improvements: yes (but check if the documentation needs to be tweaked to acknowledge the bugs and limitations in the older code).
Add ASN.1 unit tests. Cover all functions from the asn1 parse and write modules. Fix #76. Fix a couple of minor bugs and improve the documentation. This PR breaks mbedtls because two test cases in `test_suite_x509_parse` make assumptions about bugs in the ASN.1 module which this PR fixes. This PR must land in mbedtls only once https://github.com/ARMmbed/mbedtls/pull/2488 is merged there. Once this PR is merged, a commented-out test case that https://github.com/ARMmbed/mbedtls/pull/2488 adds to `test_suite_x509_parse.data` can be uncommented. Backports: * ASN.1 parsing bug fixes: I don't think so. The bugs I fixed are minor. They cause valid but uncommon data to be rejected. If it hasn't bothered anyone in all that time, it's best not to take any risks. * New tests: no. In earlier versions, ASN.1 was tested indirectly through X.509. The ASN.1 tests improve the coverage, but I don't think it's worth backporting them * Documentation improvements: yes (but check if the documentation needs to be tweaked to acknowledge the bugs and limitations in the older code).
Patater commented 2019-09-10 10:42:29 -05:00 (Migrated from github.com)

Needs rebase to resolve conflicts

Needs rebase to resolve conflicts
k-stachowiak (Migrated from github.com) requested changes 2019-09-20 09:14:16 -05:00
k-stachowiak (Migrated from github.com) left a comment

In general the changes look good. If I'm not mistaken some test cases are applied with incorrect arguments (see comments), plus, I've managed to notice some typos. Otherwise looks good.

In general the changes look good. If I'm not mistaken some test cases are applied with incorrect arguments (see comments), plus, I've managed to notice some typos. Otherwise looks good.
include/mbedtls/asn1.h
@ -176,119 +176,203 @@ mbedtls_asn1_named_data;
* \brief Get the length of an ASN.1 element.
k-stachowiak (Migrated from github.com) commented 2019-09-12 04:28:18 -05:00

Typo: "lengtth"

Typo: "lengtth"
k-stachowiak (Migrated from github.com) commented 2019-09-12 04:35:48 -05:00

Preexisting: Shouldn't this be "Updates to the ..."? Also, there's no period at the end of the previous line.

Preexisting: Shouldn't this be "Update**s** to the ..."? Also, there's no period at the end of the previous line.
include/mbedtls/asn1.h
@ -212,2 +226,2 @@
* \param end End of data
* \param val The variable that will receive the value
* \param p On entry, \c *p points to the start of the ASN.1 element.
* On successful completion, \c *p points to the first byte
k-stachowiak (Migrated from github.com) commented 2019-09-12 04:29:08 -05:00

I wonder if we can at least guarantee *p to remain in the <begin, end> range. If so, the API user can e.g. dump <*p, end> for debugging purposes.

I wonder if we can at least guarantee `*p` to remain in the <begin, end> range. If so, the API user can e.g. dump <*p, end> for debugging purposes.
tests/suites/test_suite_asn1parse.data
@ -0,0 +1,438 @@
Empty length
k-stachowiak (Migrated from github.com) commented 2019-09-20 05:24:45 -05:00

All the three free_named_data_list() are given an argument 0

All the three `free_named_data_list()` are given an argument `0`
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-20 14:04:53 -05:00
tests/suites/test_suite_asn1parse.data
@ -0,0 +1,438 @@
Empty length
gilles-peskine-arm (Migrated from github.com) commented 2019-09-20 14:04:53 -05:00 
Free named data list (1) .......................................... Segmentation fault (core dumped)

Having the intended test data revealed a bug in the test code.

``` Free named data list (1) .......................................... Segmentation fault (core dumped) ``` Having the intended test data revealed a bug in the test code.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-09-20 14:11:06 -05:00
include/mbedtls/asn1.h
@ -212,2 +226,2 @@
* \param end End of data
* \param val The variable that will receive the value
* \param p On entry, \c *p points to the start of the ASN.1 element.
* On successful completion, \c *p points to the first byte
gilles-peskine-arm (Migrated from github.com) commented 2019-09-20 14:11:06 -05:00

We can, but how useful is that? *p may be beyond the location of the error. I don't want to guarantee anything about where *p is in relation to the error. And “location of the error” may not be meaningful anyway.

For debugging purposes, looking around *p may be helpful, but then you can see in your debugger that *p points somewhere inside the buffer. For logging, I don't think this is useful. So I'd rather keep it simple and flexible: it's undefined (for example, one day we may want to set *p = NULL to avoid accidentally using some misparsed data).

We can, but how useful is that? `*p` may be beyond the location of the error. I don't want to guarantee anything about where `*p` is in relation to the error. And “location of the error” may not be meaningful anyway. For debugging purposes, looking around `*p` may be helpful, but then you can see in your debugger that `*p` points somewhere inside the buffer. For logging, I don't think this is useful. So I'd rather keep it simple and flexible: it's undefined (for example, one day we may want to set `*p = NULL` to avoid accidentally using some misparsed data).
k-stachowiak (Migrated from github.com) reviewed 2019-09-23 08:51:59 -05:00
k-stachowiak (Migrated from github.com) left a comment
No description provided.
Thanks for addressing my comments. I didn't manage to find further issues.
Patater (Migrated from github.com) reviewed 2019-10-04 03:49:56 -05:00
include/mbedtls/asn1.h
@ -176,119 +176,203 @@ mbedtls_asn1_named_data;
* \brief Get the length of an ASN.1 element.
Patater (Migrated from github.com) commented 2019-09-30 09:46:59 -05:00

This typo is still present and should be fixed.

This typo is still present and should be fixed.
gilles-peskine-arm commented 2019-10-04 04:36:36 -05:00 (Migrated from github.com)

https://github.com/ARMmbed/mbedtls/pull/2488 has been merged, so this PR is safe to merge.

https://github.com/ARMmbed/mbedtls/pull/2488 has been merged, so this PR is safe to merge.
gilles-peskine-arm (Migrated from github.com) reviewed 2019-10-04 04:37:12 -05:00
include/mbedtls/asn1.h
@ -176,119 +176,203 @@ mbedtls_asn1_named_data;
* \brief Get the length of an ASN.1 element.
gilles-peskine-arm (Migrated from github.com) commented 2019-10-04 04:37:12 -05:00

Oh, I hadn't spotted it was present twice. I amended the typo-fixing commit to also fix this one.

Oh, I hadn't spotted it was present twice. I amended the typo-fixing commit to also fix this one.
Patater (Migrated from github.com) approved these changes 2019-10-04 05:34:45 -05:00
Patater (Migrated from github.com) left a comment

LGTM

LGTM
k-stachowiak (Migrated from github.com) approved these changes 2019-10-04 06:03:12 -05:00
Patater commented 2019-10-04 06:29:49 -05:00 (Migrated from github.com)

CI failure is only TLS's check generated files test. The generated files will be fixed when TLS updates to the new version of the Crypto submodule.

******************************************************************
* check_generated_files: Check: freshness of generated source files 
* Fri Oct  4 10:27:21 UTC 2019
******************************************************************
'library/error.c' was either modified or deleted by 'scripts/generate_errors.pl'
^^^^check_generated_files: Check: freshness of generated source files: tests/scripts/check-generated-files.sh -> 1^^^^
CI failure is only TLS's check generated files test. The generated files will be fixed when TLS updates to the new version of the Crypto submodule. ``` ****************************************************************** * check_generated_files: Check: freshness of generated source files * Fri Oct 4 10:27:21 UTC 2019 ****************************************************************** 'library/error.c' was either modified or deleted by 'scripts/generate_errors.pl' ^^^^check_generated_files: Check: freshness of generated source files: tests/scripts/check-generated-files.sh -> 1^^^^ ```
Sign in to join this conversation.
Reviewers
No reviewers
Patater
k-stachowiak
Labels
Clear labels   
api-spec
Issue or PR about the PSA specifications

  
bug
Something isn't working

  
CLA requested

   
CLA valid

   
compliance
Discrepancy between the library and the PSA Crypto API specification

  
DO NOT MERGE
The PR is not intended to be merged (yet). Usually used for a review of worked in progress.

  
duplicate
This issue or pull request already exists

  
enhancement
New feature or request

  
feature branch
This issue or PR is for a short-term feature branch which doesn't have its own label.

  
good first issue
Good for newcomers

  
HEADER FORMATTING ISSUE

   
help wanted
Extra attention is needed

  
Information

   
invalid
This doesn't seem right

  
Jira status: AWAITING REVIEW

   
Jira status: CLOSED

   
Jira status: DONE

   
Jira status: IN PROGRESS

   
Jira status: IN REVIEW

   
Jira status: OPEN

   
Jira status: REOPENED

   
Jira status: RESOLVED

   
mirrored

   
mirror internally

   
needs: backports
Needs backports to Mbed TLS branches

  
needs: ci
Needs a passing full CI run

  
needs: design review

   
needs: preceding PR
Requires another PR to be merged first

  
needs: review
The pull request is ready for review. This generally means that it has no known issues.

  
needs: work
The pull request needs rework before it can be merged.

  
notabug
The bug is not considered to be a defect, but by design.

  
open for community contribution

   
Product Backlog

   
question
Further information is requested

  
ready for merge
Design and code approved, CI passed, and likewise for backports. Label added by gatekeepers only.

  
security
Security expertise requested in review

  
stale
This PR is not being actively worked on. It can be considered abandoned work.

  
type: bug

   
type: enhancement

   
type: question

   
web pages
The /docs directory in the master branch is shown on https://armmbed.github.io/mbed-crypto

  
wontfix
The cost of fixing the bug or managing it in the bug list is not justified.

  
writing
Documentation wording and formatting

No labels
api-spec
bug
CLA requested
CLA valid
compliance
DO NOT MERGE
duplicate
enhancement
feature branch
good first issue
HEADER FORMATTING ISSUE
help wanted
Information
invalid
Jira status: AWAITING REVIEW
Jira status: CLOSED
Jira status: DONE
Jira status: IN PROGRESS
Jira status: IN REVIEW
Jira status: OPEN
Jira status: REOPENED
Jira status: RESOLVED
mirrored
mirror internally
needs: backports
needs: ci
needs: design review
needs: preceding PR
needs: review
needs: work
notabug
open for community contribution
Product Backlog
question
ready for merge
security
stale
type: bug
type: enhancement
type: question
web pages
wontfix
writing
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-ARMmbed/mbed-crypto!75
No description provided.