Skip to content

Add length check for JWK key import #428

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.

Sign up for GitHub

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

Open
devgianlu wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add length check for JWK key import #428

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions spec/Overview.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10832,6 +10832,30 @@ <h5>Import Key</h5>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
Comment on lines +10835 to +10840
Copy link
Copy Markdown
Member

@twiss twiss Mar 19, 2026

Choose a reason for hiding this comment

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

I think this text is a bit in an awkward middle state where it has a lot of detailed steps, but still doesn't quite state exactly what to do; particularly this step is a bit handwavy in referring to RFC8037. I'm not opposed to being handwavy, but then I'd like to be more succinct as well. The step before this could also be argued to cover this already. Perhaps we can just make that one a bit more explicit by stating something like

                                  If |jwk| does not meet the requirements of
                                  the JWK private key format described in Section 2
                                  of [[RFC8037]], or the encoded key material does not
                                  represent an Ed25519 key of the correct length,
                                  then [= exception/throw =] a {{DataError}}.

and call it a day? (And similarly below.)

Copy link
Copy Markdown
Member

@twiss twiss Mar 19, 2026

Choose a reason for hiding this comment

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

Btw, we might want to say something similar for the spki and pkcs8 formats.
If we have to add (more) detailed parsing steps for all of them, I think it'll get a bit unwieldy.

Copy link
Copy Markdown
Member Author

@devgianlu devgianlu Mar 20, 2026

Choose a reason for hiding this comment

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

I agree it's a bit handwavy, but the spec is generally pretty explicit about the length of things, and I feel like it would a still ambigous when the JWK contains both public and private key material. For example, if the JWK is a private key you need to check both the public and private keys indipendently. So does the WPT test (see Bad key length: importKey(jwk).

Perhaps we can change interpreted to decoded: Let jwkPublic be the x field of jwk decoded according to Section 2 of [RFC8037].

There is no need to have length checks for SPKI and PKCS8 becuse the length is already encoded in the format so parsing will fail for invalid length data. Raw has already a very specific length check.

<li>
<p>
If the [= length in bits =] of |jwkPublic| is not 256,
then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the [= length in bits =] of |jwkPrivate| is not 256,
then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -10857,6 +10881,18 @@ <h5>Import Key</h5>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the [= length in bits =] of |jwkPublic| is not 256,
then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down Expand Up @@ -11684,6 +11720,30 @@ <h5>Import Key</h5>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the [= length in bits =] of |jwkPublic| is not 256,
then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPrivate| be the {{JsonWebKey/d}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the [= length in bits =] of |jwkPrivate| is not 256,
then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand All @@ -11709,6 +11769,18 @@ <h5>Import Key</h5>
of [[RFC8037]], then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |jwkPublic| be the {{JsonWebKey/x}} field of |jwk| interpreted
according to Section 2 of [[RFC8037]].
</p>
</li>
<li>
<p>
If the [= length in bits =]
of |jwkPublic| is not 256, then [= exception/throw =] a {{DataError}}.
</p>
</li>
<li>
<p>
Let |key| be a new {{CryptoKey}} object that represents the
Expand Down