Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PEM file with multiple certificates #736

Merged
merged 4 commits into from Aug 22, 2023
Merged

Conversation

HoxhaEndri
Copy link
Member

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    All the certificates inside a single PEM file will be added to "$CERT_LOG", not just the first one.
    openssl storeutl is the command doing the magic: https://www.openssl.org/docs/man1.1.1/man1/openssl-storeutl.html.

  • What is the current behavior? (You can also link to an open issue here)
    Just the first certificate of a PEM file gets stored.

  • What is the new behavior (if this is a feature change)? If possible add a screenshot.
    All certificates are stored

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • Other information:
    fixes S60 Module - .pem files not getting iterated through for cert check #708

@HoxhaEndri HoxhaEndri added bug Something isn't working Core modules (Sxx) The core scanning modules (Sxx modules) labels Aug 3, 2023
@m-1-k-3
Copy link
Member

m-1-k-3 commented Aug 3, 2023

@levi-blodgett could you test this PR with your firmware?

@levi-blodgett
Copy link

@m-1-k-3 Sorry for the late response, didn't see this for a while.

It seems to grab the cert files and some of the certs inside of the .pem files but doesn't parse the certs for the expiration dates properly, it still shows just the date of the first cert and formats off of that. Additionally, I am not confident it is parsing all certs inside of a file, mainly because after I made the bug report I found out there are also .crt files that can hold >1 cert, maybe this fork should be updated to just use the openssl storeutl regardless of file type?

Two things to note about the output when using openssl storeutl:

  1. It lists certificates like this, which could be updated to be better pretty easily:
0: Certificate
Certificate:
    Data:
...
1: Certificate
Certificate:
    Data:
...
2: Certificate
Certificate:
    Data:
  1. The one file it seemed to parse as outdated actually doesn't show any info inside of the html-report when clicked on.

I created a fork that works and will show each cert properly with their expiration dates, add the first 5 hexadecimal values from the signature, but the code is much more complex and less elegant than this solution as I didn't know that openssl storeutl existed. Perhaps a medium could be reached? I will attach screenshots of how each looks when ran on the same firmware.

The fork is at:
https://github.com/levi-blodgett/emba

The two changed files on my fork are:
https://github.com/levi-blodgett/emba/blob/master/modules/S60_cert_file_check.sh
https://github.com/levi-blodgett/emba/blob/master/modules/F50_base_aggregator.sh

These are the screenshots of the results of each run, using this fork and then on my fork:
Screenshot 2023-08-10 at 10 44 02 AM
Screenshot 2023-08-10 at 10 45 05 AM
Screenshot 2023-08-10 at 10 45 25 AM
Screenshot 2023-08-10 at 10 45 41 AM

Please let me know if I can assist further, thanks.

@HoxhaEndri
Copy link
Member Author

HoxhaEndri commented Aug 11, 2023

@levi-blodgett Thank you for your detailed reply! I will look into it.

@m-1-k-3 m-1-k-3 marked this pull request as draft August 15, 2023 13:53
@m-1-k-3
Copy link
Member

m-1-k-3 commented Aug 15, 2023

@HoxhaEndri drafted it, so you can finish the work before reviewing it again.

@HoxhaEndri
Copy link
Member Author

@levi-blodgett I used openssl storeutl and kept its formatting, because I do not find it bad that each nested certificate gets an index. For the signature, I just keep the first line and exactly as you, I check the certificates that could expire within 2 years, however I do not know which time period would be the best to use. Now it should work like expected.

@m-1-k-3 m-1-k-3 marked this pull request as ready for review August 22, 2023 10:21
@m-1-k-3 m-1-k-3 merged commit 00f1030 into e-m-b-a:master Aug 22, 2023
12 checks passed
@m-1-k-3
Copy link
Member

m-1-k-3 commented Aug 22, 2023

@levi-blodgett and @HoxhaEndri looks really good. Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Core modules (Sxx) The core scanning modules (Sxx modules)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

S60 Module - .pem files not getting iterated through for cert check
4 participants