Skip to content

Conversation

@skmcgrail
Copy link
Member

Issues:

Resolves P342458791

Description of changes:

Adds basic support for openssl asn1parse with the following arguments:

-in <inputFile>
-inform (PEM | DER)

The behavior of the command will default to OpenSSL's -strictpem flag behavior which was an optional feature. We can revisit if this determined to be required in order to relax the input restriction for valid PEM blocks.

Call-outs:

The original asn1parse function used ASN1_dump rather then ASN1_parse, under the hood they are the same function, except that ASN1_dump can be given a flag to indicate that unknown data should be hex dumped out. As our BIO hexdump functions doesn't match OpenSSL's the output would be slightly different. For now I've opted to not support the -dump flag for the CLI, and chose to only expose ASN1_parse in the library. That way we could come back and add -dump and ASN1_dump with proper output without worrying about backwards compatibility concerns.

Testing:

  • Added a series of corpus files for some BER and DER encodings (the asn1parse tool / library components due allow some BER features e.g. indefinite length encoding).
  • Used DER corpus files to seed a fuzzer for testing the ASN1_parse library function.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@skmcgrail skmcgrail requested a review from a team as a code owner December 5, 2025 20:53
@skmcgrail skmcgrail changed the title Asn1parse [tool-openssl] basic asn1parse support Dec 5, 2025
@codecov-commenter
Copy link

codecov-commenter commented Dec 5, 2025

Codecov Report

❌ Patch coverage is 65.51724% with 110 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.21%. Comparing base (04b2db3) to head (2683105).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crypto/asn1/asn1_par.c 65.51% 110 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2882      +/-   ##
==========================================
- Coverage   78.26%   78.21%   -0.05%     
==========================================
  Files         683      685       +2     
  Lines      117604   118083     +479     
  Branches    16515    16620     +105     
==========================================
+ Hits        92042    92358     +316     
- Misses      24675    24839     +164     
+ Partials      887      886       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@skmcgrail skmcgrail requested a review from justsmth December 9, 2025 23:52
return 1;
}

static int asn1_parse_string_type(BIO *bp, const uint8_t *data, long len) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How paranoid should we be about the string. There are several different encodings that this could be, some of which could have an invalid length (e.g., multi-byte UTF8) or contain control characters.

Should we basically have this behave similar to asn1_parse_octet_string_type in relation to validating ASCII content?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function entry point is guarded by a check on the explicit ASN.1 types that are either numbers, digits, ascii, or subset of ascii, and UTF-8, which appears to be assumed that the output destination for the BIO, be that a terminal interface or a file supports UTF-8 encoding. I've checked OpenSSL 3+ and the behavior is the same there as well.

justsmth
justsmth previously approved these changes Dec 11, 2025
samuel40791765
samuel40791765 previously approved these changes Dec 11, 2025
@skmcgrail skmcgrail dismissed stale reviews from samuel40791765 and justsmth via 2683105 December 11, 2025 23:48
@skmcgrail skmcgrail enabled auto-merge (squash) December 12, 2025 00:25
@skmcgrail skmcgrail merged commit ec39cb3 into aws:main Dec 12, 2025
387 of 393 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants