1035018 - nsIDataSignatureVerifier returns undocumented NS_ERROR_FAILURE rather than returning true/false

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect)

Description

[Zack]

Attachment: More details about the problem.

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140605174243

Steps to reproduce:

In short, I have been working on a mechanism that will download a file after first downloading and inspecting a JSON file that describes changes to the former file.  Among other things, the mechanism downloads a file containing the signature of the JSON file's hashed (with SHA256) contents, hashes the JSON file's contents itself, and then uses nsIDataSignatureVerifier to verify the downloaded signature using a hardcoded public key.

The  whole process of creating this JSON file, generating a private key, signing the digest of the JSON file generated, and verifying this signature are outlined on the following page:
https://github.com/redwire/https-everywhere/blob/rulesetUpdating/doc/updateJSONSpec.md#updatejson-and-updatejsonsig
The data I've generated with the process outlined above is available at:
https://github.com/redwire/https-everywhere/tree/rulesetUpdating/utils/testing/sign_verify
The instructions for running the test suite we have are at:
https://github.com/redwire/https-everywhere/tree/feature/tests/https-everywhere-tests
And finally, the tests I have written are at:
https://github.com/redwire/https-everywhere/blob/feature/tests/https-everywhere-tests/test/test-rsupdate-verify.js


Actual results:

NS_ERROR_FAILURE is returned by the call to verifyData.


Expected results:

verifyData should have returned the value `true`.

Comment 1

[Benjamin Smedberg]

Not an XPCOM bug, the implementation is in PSM:
http://hg.mozilla.org/mozilla-central/annotate/1dc6b294800d/security/manager/ssl/src/nsDataSignatureVerifier.cpp#l40

There are several exit paths from the function:

If base-64 decoding of the key fails, it throws at line 58

If it can't extract the public key from the data, it throws, at lines 65 and 73

If it can't base-64 decode the signature, it throws at line 84

If it can't convert the signature data into a signature, it throws at line 96

It returns true or false based on whether the data matches the signature, at line 111.

I don't know whether those early failures should return false instead of throwing, but that's how it currently works. In either case I suspect it should be documented better. I suggest brian smith or david keeler as possible reviewers.

Comment 2

[:Cykesiopka]

David: any thoughts on whether we should change the implementation to not throw, or just update the documentation?

Comment 3

[Dana Keeler (she/her) [:keeler]]

Unless I'm misremembering, the implementations of most of these interfaces (I'm thinking nsIX509CertDB, etc.) throw if they encounter failures like these. I don't think we need to change any implementations here. Documentation might be helpful, but it might just boil down to "calling these functions may throw if given invalid/unexpected input". Since this has sat for a year and a half, I'm inclined to wontfix it.

Comment 4

[:Cykesiopka]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #3)
> Unless I'm misremembering, the implementations of most of these interfaces
> (I'm thinking nsIX509CertDB, etc.) throw if they encounter failures like
> these. I don't think we need to change any implementations here.
> Documentation might be helpful, but it might just boil down to "calling
> these functions may throw if given invalid/unexpected input". Since this has
> sat for a year and a half, I'm inclined to wontfix it.

Eh, I guess I'm OK with WONTFIX.