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

[jws] check signature length #1004

Merged
merged 6 commits into from
Oct 26, 2023
Merged

Conversation

lestrrat
Copy link
Collaborator

No description provided.

We have been notified that certain JWS messages with ES256 signatures
come with 63 byte signatures instead of the desired 64, and that
jwx successfully verifies these. Upond digging, we have found that
PR #65 removed the previous check for the key sizes, way back in 2019.

Without this check it is possible for an unpadded R value to pass
verification. For example, R with 31 bytes worth of data without
padding followed by S with 32 bytes can pass the verification
because when generating math.BigInt produces a valid result.

For example:

  Regular Signagure -> | R (32 bytes)... | S (32 bytes)... |

  Invalid Signature -> | R (31 bytes)... | S (32 bytes)... |

The type of signature that will get past is a signature where,
both values of R and S are valid but R is not padded at byte 0.

Since the signature content must match the correct content either
way, it is highly unlikely that this can be used for anything other
than adding an extra byte at the end of such a signature (that is,
the end user who produced the signature must already have the
correct signature anwyays).

However, omitting the check _does_ allow otherwise invalid signature
to be verified correctly.
@lestrrat lestrrat changed the title Jws check signature length [jws] check signature length Oct 26, 2023
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bd67589) 72.29% compared to head (6c5d2ed) 72.28%.

Additional details and impacted files
@@              Coverage Diff               @@
##           develop/v2    #1004      +/-   ##
==============================================
- Coverage       72.29%   72.28%   -0.01%     
==============================================
  Files              93       93              
  Lines           13644    13650       +6     
==============================================
+ Hits             9864     9867       +3     
- Misses           2971     2973       +2     
- Partials          809      810       +1     
Files Coverage Δ
internal/ecutil/ecutil.go 87.30% <100.00%> (+0.41%) ⬆️
jws/ecdsa.go 79.71% <100.00%> (-1.64%) ⬇️

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

@lestrrat lestrrat merged commit 1ecc78f into develop/v2 Oct 26, 2023
31 of 32 checks passed
@lestrrat lestrrat deleted the jws-check-signature-length branch October 26, 2023 22:27
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.

1 participant