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.

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

[WebCryptoAPI] use assert_precondition to avoid spurious failures #20176

Closed
wants to merge 1 commit into from
Closed

[WebCryptoAPI] use assert_precondition to avoid spurious failures #20176

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
2 changes: 2 additions & 0 deletions WebCryptoAPI/generateKey/successes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ function run_test(algorithmNames, slowTest) {
assert_goodCryptoKey(result, algorithm, extractable, usages, "secret");
}
}, function(err) {
var supported = !err.toString().includes('not supported');
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 is testing the error message which isn't defined by the spec. Unfortunately per https://w3c.github.io/webcrypto/#SubtleCrypto-Exceptions (and algorithms) it looks like the DOMException type to check for would be "NotSupportedError" but Chromium actually uses "OperationError", so I think Chromium would need to change...

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you seeing this for an algorithm in its entirety or are you seeing this for a specific size in an algorithm. For example, if the FOO algorithm is not supported at all, then NotSupportedError is correct. However, there are discussions what the correct error would be if you do not support the key size of 192 for the AES algorithms. In this case OperationError might be considered to be a legal answer.

Copy link
Member Author

Choose a reason for hiding this comment

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

The case being hit in the test is AES-192 being explicitly not supported in Chromium (it has a dedicated error message) but the DOMException type is “OperationError”.

If there’s no per-spec way to detect this case as being unsupported (as opposed to supported but broken) then these tests can only be allowed to fail in the usual way, just like they currently are.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is correct. Several of us argued that not supported was better but Chromium was the hold out. I don't think this is going to be fixable.

Copy link
Member Author

Choose a reason for hiding this comment

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

What does a straight reading of the spec say should happen if you don’t support AES-192 but some other sizes of AES? Is it left undefined, or does it say “OperationError”?

Copy link
Contributor

Choose a reason for hiding this comment

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

So I just walked the spec again looking for anything that provides any guidance, but the real answer is that even though we knew that Chrome was doing this it was ignored.

In section 27.4 Operations:
GenerateKey - In step 4 it says that if the key generation step fails, then throw an
OperationException. It could be argued that this is what Chrome is doing.

ImportKey - for option 'raw' of step 2, there is no error if the key length is 192. For the second option it would appear that the best answer that is returned would be DataError on the assumption that the second clause of step 5 is ignored for non-support. There are no steps for ImportKey which would return an OperationError.

I would expect that the same issue should be appearing for the import/export functions as well. Looking at the code it looks like it should be.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could the spec be updated with an explicit step at some central place used by all methods that says what to do if the key length is not supported?

Also, what’s the background for AES-192 specifically not being supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

I could easily write an update that would do that, but I would have absolutely no idea how to go about getting it integrated in the document. The original working group has been closed for 3 years and the follow on group appears to have closed this year. I am not a part of the W3C in any way. (I own a winery and was an invited expert.) The person to as about this is probably @wseltzer but even that is a guess on my part. If written this could be applied to the various RSA algorithms as not everybody is going to allow support for all modulus lengths.

Going purely on memory, as I cannot find anything in my mail archive or a quick search of the minutes, is that Chrome believed that if you though 128-bits is not enough then you should just jump all of the way to 256-bits. Nobody really needed the intermediate value even if NIST had defined it to be used for some of the profiles that were defined.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh... there's no way to update the spec anymore? That's going to be a problem sooner or later, and right now actually.

@sideshowbarker @plehegar I see you both in https://github.com/w3c/webcrypto/graphs/contributors, do you know the status of this?

assert_precondition(supported, algorithm.name + ' not implemented');
assert_unreached("Threw an unexpected error: " + err.toString());
});
}, testTag + ": generateKey" + parameterString(algorithm, extractable, usages));
Expand Down