Issue 21759002: WebCrypto: Add algorithm normalization rules for RSASSA-PKCS1-v1_5.

Issue 21759002: WebCrypto: Add algorithm normalization rules for RSASSA-PKCS1-v1_5. (Closed)

Created:
7 years, 4 months ago by eroman

Modified:
7 years, 4 months ago

Reviewers:
abarth-chromium

CC:
blink-reviews, Nils Barth (inactive), jamesr, kojih, jsbell+bindings_chromium.org, eae+blinkwatch, tommyw+watchlist_chromium.org, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, haraken, Nate Chapin, jeez, do-not-use

Base URL:
svn://svn.chromium.org/blink/trunk

Visibility:
Public.

More Reviews

Description

WebCrypto: Add algorithm normalization rules for RSASSA-PKCS1-v1_5. BUG=245025 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155458

Patch Set 1 : #

Total comments: 4

Patch Set 2 : Add explicit to some constructors #

Patch Set 3 : rebase onto master #

Created: 7 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+212 lines, -52 lines)			Patch
M	LayoutTests/crypto/normalize-algorithm.html	View		7 chunks	+44 lines, -15 lines	0 comments	Download
M	LayoutTests/crypto/normalize-algorithm-expected.txt	View		2 chunks	+12 lines, -6 lines	0 comments	Download
M	Source/bindings/v8/custom/V8AlgorithmCustom.cpp	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/core/platform/chromium/support/WebCryptoAlgorithm.cpp	View		1 chunk	+14 lines, -0 lines	0 comments	Download
M	Source/modules/crypto/AesCbcParams.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/modules/crypto/AesKeyGenParams.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/modules/crypto/Algorithm.cpp	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/modules/crypto/HmacParams.cpp	View		1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/modules/crypto/NormalizeAlgorithm.cpp	View		4 chunks	+42 lines, -6 lines	0 comments	Download
A +	Source/modules/crypto/RsaKeyGenParams.h	View		1 chunk	+10 lines, -6 lines	0 comments	Download
A +	Source/modules/crypto/RsaKeyGenParams.cpp	View		1 chunk	+14 lines, -4 lines	0 comments	Download
A +	Source/modules/crypto/RsaKeyGenParams.idl	View		1 chunk	+3 lines, -2 lines	0 comments	Download
A +	Source/modules/crypto/RsaSsaParams.h	View		1 chunk	+5 lines, -5 lines	0 comments	Download
A +	Source/modules/crypto/RsaSsaParams.cpp	View		1 chunk	+5 lines, -4 lines	0 comments	Download
A +	Source/modules/crypto/RsaSsaParams.idl	View		1 chunk	+1 line, -1 line	0 comments	Download
M	Source/modules/modules.gypi	View		2 chunks	+6 lines, -0 lines	0 comments	Download
M	public/platform/WebCryptoAlgorithm.h	View		3 chunks	+7 lines, -0 lines	0 comments	Download
M	public/platform/WebCryptoAlgorithmParams.h	View	1	4 chunks	+34 lines, -3 lines	0 comments	Download

Messages

Total messages: 5 (0 generated)

Expand Messages | Collapse Messages

abarth-chromium

LGTM https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/NormalizeAlgorithm.cpp File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/NormalizeAlgorithm.cpp#newcode221 Source/modules/crypto/NormalizeAlgorithm.cpp:221: // FIXME: This is losing precision; modulusLength is ...

7 years, 4 months ago (2013-08-02 17:56:56 UTC) #2

eroman

https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/NormalizeAlgorithm.cpp File Source/modules/crypto/NormalizeAlgorithm.cpp (right): https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/NormalizeAlgorithm.cpp#newcode221 Source/modules/crypto/NormalizeAlgorithm.cpp:221: // FIXME: This is losing precision; modulusLength is supposed ...

7 years, 4 months ago (2013-08-02 19:25:42 UTC) #3

https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/Norm...
File Source/modules/crypto/NormalizeAlgorithm.cpp (right):

https://codereview.chromium.org/21759002/diff/3001/Source/modules/crypto/Norm...
Source/modules/crypto/NormalizeAlgorithm.cpp:221: // FIXME: This is losing
precision; modulusLength is supposed to be a uint32
On 2013/08/02 17:56:56, abarth wrote:
> Should we make dictionary understand uint32?  I don't think numbers that large
> are representable in JavaScript anyway.

I need to do a bit of follow-up on what WebIDL intends here.

Here is the thing: Dictionary already has methods for extracting unsigned
numbers:

  Dictionary::get(const String& key, unsigned& value);

The problem is in its implemnetation, it simply gets an Int32, and then does a
static_cast<unsigned>() on it.

So that means, if the user passed in "-1", then it would complete successfully
and return to me 0xFFFFFFFF.

I am not sure if this is what the WebCrypto spec intends, but it seems wrong to
me. I would reject negative numbers than silently convert them to large numbers
in twos complement.

I could use the existing method:
   Dictionary::get(const String& key, double& value)

And then do the sign test before converting to an "unsigned". But I wanted to do
a bit more research before I tackle this.

As I recall, JavaScript can accurately represent up to 53-bit integers using
floating point representation, so that would be fine for extracting my 32-bit
unsigned value.

Although it does seem strange to allow fractional numbers in Javascript (like a
key length of 1024.9), and then have that magically truncated to 1024.9.

Note that with the integer method I am using now, fractional numbers are also
allowed.

https://codereview.chromium.org/21759002/diff/3001/public/platform/WebCryptoA...
File public/platform/WebCryptoAlgorithmParams.h (right):

https://codereview.chromium.org/21759002/diff/3001/public/platform/WebCryptoA...
public/platform/WebCryptoAlgorithmParams.h:106: WebCryptoRsaSsaParams(const
WebCryptoAlgorithm& hash)
On 2013/08/02 17:56:56, abarth wrote:
> explicit?

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/eroman@chromium.org/21759002/12001

7 years, 4 months ago (2013-08-02 20:32:36 UTC) #4

Message was sent while issue was closed.

Change committed as 155458

Expand Messages | Collapse Messages