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 private keys detect enchancement #456

Merged
merged 12 commits into from
Dec 14, 2023
Merged

PEM private keys detect enchancement #456

merged 12 commits into from
Dec 14, 2023

Conversation

babenek
Copy link
Contributor

@babenek babenek commented Nov 18, 2023

Description

Please include a summary of the change and which is fixed.

  • "filter_type" is an optional field in rules.
  • PEM private key Elliptic Curves are found also
  • strict filtering for PEM keys (ASN1 or keyword "bcrypt")
  • Encrypted keys are skipped
  • PGP keys are checked with entropy only

How has this been tested?

Please describe the tests that you ran to verify your changes.

@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (7559f13) 90.58% compared to head (5e87de1) 90.33%.

Files Patch % Lines
credsweeper/scanner/scan_type/pem_key_pattern.py 80.00% 4 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #456      +/-   ##
==========================================
- Coverage   90.58%   90.33%   -0.26%     
==========================================
  Files         126      125       -1     
  Lines        4280     4293      +13     
  Branches      679      688       +9     
==========================================
+ Hits         3877     3878       +1     
- Misses        267      274       +7     
- Partials      136      141       +5     

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

@babenek babenek marked this pull request as ready for review November 18, 2023 15:20
@babenek babenek requested a review from a team as a code owner November 18, 2023 15:20
@babenek babenek marked this pull request as draft November 20, 2023 09:54
@babenek babenek marked this pull request as ready for review November 20, 2023 19:56
Comment on lines -389 to +332
- (?P<value>-----BEGIN\s(?!ENCRYPTED|EC)[^-]*PRIVATE[^-]*KEY[^-]*-----(.+-----END[^-]+-----)?)
filter_type:
- LineSpecificKeyCheck
- (?P<value>-----BEGIN\s(?!ENCRYPTED)[^-]*PRIVATE[^-]*KEY[^-]*-----(.+-----END[^-]+-----)?)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you explain it more about why you decided to remove EC keyword from the pattern?
I think EC(elliptic curve encryption) can't be decrypted..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In openssl/test exists not encrypted EC key https://github.com/openssl/openssl/blob/master/test/testec-p256.pem.
It can be analysed with ASN1 parser. It works without a password:

openssl dgst -sha256 -sign testec-p256.pem -out signature.bin smcont.txt 
openssl dgst -sha256 -verify testecpub-p256.pem  -signature signature.bin smcont.txt 

Encrypted key i found has structure like this and cannot be parsed with asn1:

-----BEGIN EC PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
DEK-Info: AES-128-CBC,692.....

BXv....

So, i think, with the simple analysis for ASN1 structure we can decide whether a key is encrypted or not in the level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh.. i didn't know about that.
Yes, using ASN1 decryption can be a solution.
How about change the logic to check ASN1 decryptable if the line includes EC keyword?
And other cases that doesn't include EC keyword just follow the logic before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Full decryption is not acceptable for obfuscated benchmark. If first symbols are not changed - the header might be parsed in benchmark.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@csh519
EC keys are ASN1 too (if are not encrypted), so the approach may be used too.
OPENSSH keys have PBKDF2 format and cannot be checked with the obfuscated values. Only keyword 'bcrypt' in header may point to encrypted key.
PGP is still checked for entropy..

@babenek babenek marked this pull request as draft November 21, 2023 06:46
@babenek babenek force-pushed the ecpkey branch 7 times, most recently from b3d0d86 to 5674c5f Compare December 5, 2023 15:02
@babenek babenek marked this pull request as ready for review December 5, 2023 18:14
@babenek babenek marked this pull request as draft December 5, 2023 18:14
credsweeper/rules/config.yaml Outdated Show resolved Hide resolved
@babenek babenek marked this pull request as ready for review December 6, 2023 19:17
@babenek babenek changed the title Remove ignoring elliptic curves private keys PEM private keys detect enchancement Dec 7, 2023
Yullia
Yullia previously approved these changes Dec 13, 2023
kmnls
kmnls previously approved these changes Dec 13, 2023
Copy link
Contributor

@kmnls kmnls left a comment

Choose a reason for hiding this comment

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

Agree

@babenek babenek dismissed stale reviews from kmnls and Yullia via a508383 December 13, 2023 16:21
Copy link
Contributor Author

@babenek babenek left a comment

Choose a reason for hiding this comment

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

TODO: rollback benchmark reference before merge

.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
@babenek babenek requested a review from kmnls December 14, 2023 07:09
@babenek
Copy link
Contributor Author

babenek commented Dec 14, 2023

@csh519 @kmnls @Yullia @xDizzix @iuriimet
please approve Samsung/CredData#52 first

Rollback custom benchmark
@babenek babenek requested a review from Yullia December 14, 2023 07:59
@babenek babenek merged commit fe909f9 into Samsung:main Dec 14, 2023
29 checks passed
@babenek babenek deleted the ecpkey branch December 14, 2023 08:25
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.

6 participants