1235768 - window.crypto.subtle.encrypt - issue encrypting a string longer than 126 characters

Status: RESOLVED INVALID

(Core :: DOM: Security, defect)

Description

[u496394]

Attachment: generateKeyAndEncrypt.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151223140742

Steps to reproduce:

Tried to create a sample to encrypt text using an asymetric key-pair but run into an issue when working with longer strings. I tried the following:
- created an asymetric key-pair using the WebCrypto API's generateKey function
- tried to encrypt sample string using the public key generated

To give it a try, I added the sample source here. Maybe I missed something?


Actual results:

- first sample (function encryptWorking()) trying to encrypt text of 126 characters working well
- second sample (function encryptNotWorking()) trying to encrypt string of 127 characters and more running into an issue


Expected results:

- second sample (function encryptNotWorking()) trying to encrypt string of 127 characters and more should also working well

Comment 1

[Anders Rundgren]

I haven't studied the code in detail but RSA encryption is limited to the length of the key so in practice it is only used for encrypting symmetric keys which have no limits.

That is, I don't think this is a bug.

Comment 2

[u496394]

Hi.

Thanks for the note. The Developer Console doesn't provide any hint for it and I'm not really sure what concerns the length. So far I couldn't find anything concerning this problem.

Comment 3

[Dirkjan Ochtman (:djc)]

Per the spec: "Developers making use of the SubtleCrypto interface are expected to be aware of the security concerns associated with both the design and implementation of the various algorithms provided. The raw algorithms are provided in order to allow developers maximum flexibility in implementing a variety of protocols and applications, each of which may represent the composition and security parameters in a unique manner that necessitate the use of the raw algorithms."

I've added this warning to MDN; I'm not sure where you learned about this API, but you might want to alert them, too. However, providing a warning in the console might be useful.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

With RSA-OAEP the maximum message length you can encrypt is:

m - 2 - 2*hLen

With m=2048 and hLen=512 as in your example that is:

2048 - 2 - 2*512 = 1022 bits

Although 127*8=1016 bits (and thus a little smaller) this very likely hits the maximum message length. I would suggest you use RSA only to encrypt small messages, i.e. a symmetric AES key and use that to bulk-encrypt data.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That said, we could probably do better in terms of the exception we reject with than OperationError with no indication as to why, no?  Seems like it would be a good idea if it's not hard.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #5)
> That said, we could probably do better in terms of the exception we reject
> with than OperationError with no indication as to why, no?  Seems like it
> would be a good idea if it's not hard.

So... how would we do that? The spec is rather clear saying "If performing the operation results in an error, then throw an OperationError." There's no check of the message size before that.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Sure.  My question is whether we, at the point when we throw the OperationError, know why the operation resulted in an error.

Because what we can do is throw an OperationError whose .message is any string we want; the spec does not define the contents of .message, right?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #7)
> Sure.  My question is whether we, at the point when we throw the
> OperationError, know why the operation resulted in an error.

PK11_PubEncrypt() fails but I think it doesn't exactly tell us why. We could definitely check the message length ourselves before calling though.

> Because what we can do is throw an OperationError whose .message is any
> string we want; the spec does not define the contents of .message, right?

You're right, we should fail with a better error message. Problem is that the WebCrypto code doesn't support any custom error messages afaict:

https://hg.mozilla.org/mozilla-central/annotate/29258f59e545/dom/crypto/WebCryptoTask.cpp#l354

And also it seems that the spec does define the error message itself? The message is optional though so probably we could also change it to what we like:

http://www.w3.org/TR/WebCryptoAPI/#SubtleCrypto-Exceptions

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Problem is that the WebCrypto code doesn't support any custom error messages afaict:

OK.  Maybe worth changing, but moderately annoying.

> And also it seems that the spec does define the error message itself?

Yeah, I'm not sure what the spec is doing here.  This was explicitly raised as a spec issue during the feedback period as I recall, and it looks like they didn't address it.  This section is so unclear as to what it means that I feel like we can do anything we feel like, especially since the issue _was_ raised in the past and ignored....

Comment 10

[Ryan Sleevi]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #9)
> > Problem is that the WebCrypto code doesn't support any custom error messages afaict:
> 
> OK.  Maybe worth changing, but moderately annoying.

Unfortunately, it's not something that can be reliably specced, because it's entirely up to the crypto library whether or not you get details.

The overall goal - which came, in strong part, due to the insistence by folks in the WebCrypto WG that we be agnostic enough to support smart cards (which was half-committed to by Mozilla, but then half-not), means that the only reliable error we can pass from the crypto library is OperationError. Maybe the library has details - maybe not. Many don't. And when they do, there's not a guarantee that it will be a 1:1 mapping between Crypto libraries; this isn't a matter of calling function foo.dll!Foo in Mozilla and bar.dll!Foo in Chrome - this is radically different approaches in the library, which cannot be easily smoothed.

Webcrypto reflects that.

> > And also it seems that the spec does define the error message itself?
> 
> Yeah, I'm not sure what the spec is doing here.  This was explicitly raised
> as a spec issue during the feedback period as I recall, and it looks like
> they didn't address it. 

It was explicitly chosen not to be addressed. Which is to say, it was addressed, and rejected.

> This section is so unclear as to what it means that
> I feel like we can do anything we feel like, especially since the issue
> _was_ raised in the past and ignored....

This is not an accurate statement. It was not ignored. As explained above, the ability to reliably explain errors when interacting with the third-party libraries (whether they be CommonCrypto, CryptoAPI, NSS, or BoringSSL - four separate implementations shipping today) is non-existent. The only way to resolve this is to do processing in the browser, before delegating to the library. That carries with it some degree of security risk, but also significant complexity. Considering that Mozilla has been entirely non-responsive to repeated requests to help find a solution to solve something with *minimal* security risks (processing of SPKI within exportKey and importKey), I don't think it's really fair or helpful to suggest that this issue is ignored in WebCrypto.

I'd love to find a solution that could work, and if there's agreement that everyone needs to implement more crypto-specific logic in the browser - something that Microsoft and Safari were previously (and reasonably) opposed to - then we can easily find a path forward. If it's something Mozilla would like to work on, there's already stuff waiting for feedback in the WG which is completely blocking interoperability and spec progress, so it'd be great to start with that first :)

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Unfortunately, it's not something that can be reliably specced

Sure.  I'm not asking for it to be specced.  I'm saying we should consider having our console say something useful, by setting a useful .message on the OperationError.

> Which is to say, it was addressed, and rejected.

What was rejected?  Making the spec actually be clear about what it's saying?

My issue with that spec section is that it's not clear about what it's trying to say.  Is it saying that whenever throwing OperationError the .message must be precisely the string "The operation failed for an operation-specific reason"?  Is it saying that the string is up to the UA?  Is it saying that it's up to the UA but if the UA has no idea then "The operation failed for an operation-specific reason" is a good default?  Something else?

I totally understand that the spec can't require .message to usefully describe the roots of the failure.  My problem is that it's not clear from the spec whether .message is _allowed_ to describe those roots.  And the vagueness of this spec section was definitely brought up before, and it's not clear to me why it's still vague.

Comment 12

[Ryan Sleevi]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #11)
> What was rejected?  Making the spec actually be clear about what it's saying?

The concern was that .message becomes part of the API contract and thus a cause of non-interoperability. For example, Edge might have a .message of "Error 0xDEADBEEF" which might turn out to be E_BUFFER_TOO_LARGE, NSS would have CKR_DATA_INVALID (although NSS will swallow that into general crap like CKR_FUNCTION_FAILED - effectively the same problem here), and BoringSSL would just have "Failed".

That said, Blink totally *does* provide details in .message - https://code.google.com/p/chromium/codesearch#chromium/src/components/webcrypto/status.cc&rcl=1451997057&l=1 to see what we do.

> My issue with that spec section is that it's not clear about what it's
> trying to say.  Is it saying that whenever throwing OperationError the
> .message must be precisely the string "The operation failed for an
> operation-specific reason"?  Is it saying that the string is up to the UA? 
> Is it saying that it's up to the UA but if the UA has no idea then "The
> operation failed for an operation-specific reason" is a good default? 
> Something else?

So now this is clearer - your concern isn't about throwing OperationError (which has come up several times in the group), this is about what the contents of .message would be. I responded towards the notion of special errors beyond OperationError, which the spec originally had and were all smoothed into OperationError.

The table in http://w3c.github.io/webcrypto/Overview.html#SubtleCrypto-Exceptions was meant to mirror that from IndexedDB (the old W3C version) - http://www.w3.org/TR/IndexedDB/#exceptions - but it looked like that one got language to make it clear .message was optional. The newer IndexedDB makes it even clearer the expectation for .message being implementation specific http://w3c.github.io/IndexedDB/#exceptions .

Feel free to file a spec bug to align the language there, although I don't think the New-IDB approach will appease the W3C overlords - Harry went on a killing spree of removing all "Notes" with the view that they all need to be purged from the spec. So probably language like Old-IDB?

> I totally understand that the spec can't require .message to usefully
> describe the roots of the failure.  My problem is that it's not clear from
> the spec whether .message is _allowed_ to describe those roots.  And the
> vagueness of this spec section was definitely brought up before, and it's
> not clear to me why it's still vague.

Well, for a number of cases - including the one in this post - we *could* provide more details. If the UA looked at the inputs, looked at its algorithm-specific knowledge (e.g. this is OAEP and the key size is X bits), then it could pre-process the inputs. At one point I even had language to do just that - but then it gets into the problem of how much (or how little) the UA should know about a given algorithm and its constraints. Personally, I'd love to give the most actionable feedback to developers, with a Strong API guarantee.

To the question at hand, which is "Is the intention of the spec to allow .message to provide details", the answer is Yes, and other implementations do just that. If that part of the spec isn't clear (and we just need to adopt normative language like New-IDB or Old-IDB), great - WebCrypto spec bug, bonus points if you just want to do a pull request on the GitHub, even more bonus points if you can get Mozilla folks active to help work out the interoperability issues :)

Comment 13

[Boris Zbarsky [:bzbarsky]]

> The concern was that .message becomes part of the API contract and thus a cause of non-interoperability

That's a valid concern, sure.  Rejecting that as a concern is also perfectly ok, of course.  That's not what I was talking about; I'm glad we're more on the same page now.

> Feel free to file a spec bug to align the language there

https://www.w3.org/Bugs/Public/show_bug.cgi?id=29359

> Harry went on a killing spree of removing all "Notes"

A clear description of what the second column of the table means would be much better than a Note, of course.

> the answer is Yes

Great.  Let's just fix the spec so it's clear about that.  ;)