SignedExchangeSignatureVerifier should verify original parsed representation, not re-encode it |
|||||||||
Issue descriptionSignedExchangeSignatureVerifier currently reserializes the parsed representation to verify a signature. This puts the encoder in the critical path. Instead, protocols and their implementations should verify the original serialized form directly. In X.509, it is the TBSCertificate which, thanks to DER, means encode(decode(cert)) are required to round-trip. (This is part of why canonical encodings are important for security.) TLS signs the handshake transcript which is formed from the data on the wire, and not reencoding parsed TLS messages. QUIC and Roughtime are other examples of this. Reserializing things means that mere functional bugs in the encoder (e.g. forgetting about a newly-added field) become security bugs that allow attackers to inject content into another origin. It also puts more importance on parser behavior on invalid inputs. Using the serialized form cuts out a host of possible mistakes and attacks. It is also more efficient. Code size is better (you don't even need an encoder) and you don't need to make a ton of copies to recover the original form. All this is especially nice when the parser is StringPiece-based, such as https://commondatastorage.googleapis.com/chromium-boringssl-docs/bytestring.h.html which does not allocate at all. (Unfortunately, Web Packaging used CBOR, which has some mistakes that mean it is harder to build that style of parser. DER is a much much better serialization in that respect.) I believe the specification was fixed a while ago to make a secure verifier possible. This should be reflected in the implementation. (And probably also fix the spec to more strongly require implementations do this.)
,
Jul 19
Yeah, I was trying to find it in the spec and was having a hard time too. :-) We should revise the spec to make this clearer.
,
Jul 19
Thanks for noticing!
,
Jul 20
,
Jul 30
,
Jul 30
,
Jul 30
(kinuko@ made a suggestion to dump my progress/thoughts here. here I'm trying to follow her suggestion :) The current implementation snapshot, namely b1, embeds the original representation within a CBOR map. This format makes it tricky to implement this using Chromium without encode(decode(hdrs)) using cbor_writer.cc , since we can't mix "raw cbor bytes" and unencoded "CBORValue"s. I'll follow up with a chromium WIP CL that will workaround the issue by not using cbor_writer.cc to encode the top-level map, but we probably would want to change the message encoding in the near future. [1] https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-01#section-3.5
,
Jul 30
Filed https://github.com/WICG/webpackage/issues/276 for spec chage
,
Jul 31
Thanjs Kouhei. If changing the spec further could make this easier (potentially across implementations) and Jeffrey and others are onboard I prefer waiting the spec change before changing the impl.
,
Jul 31
(I suppose we will need to cut another snapshot though if we do that)
,
Jul 31
Let me try to enumerate possible actions from here. 1. Land CL c#9 to avoid enc(dec(msg)) from b1, make spec change so avoid enc(dec(msg)) is easier, and snapshot b2. [pro] b1 will still be OT-able without security compromise. 1'. Land CL c#9 to avoid enc(dec(msg)) from b1, change spec wording to discourage enc(dec()), but keep the end result format to be the same. [pro] We don't have to snapshot b2 2. Keep b1 impl as is, make spec change and snapshot b2, Chromium will avoid enc(dec(msg)) from b2. (but will enc(dec()) when parsing b1). [pro] We may need to forbid b1 in the Origin Trials (only accept >=b2)
,
Aug 8
davidben: What would your preference be of the c#12 options?
,
Aug 13
davidben: Would you mind answering c#13? I'd really appreciate your input here since this affects if we will make b2 cut or not.
,
Aug 14
Hrm. I see. Doing things with sub-elements in DER is straightforward because DER got length prefixes right. But CBOR messed this up, which makes this is actually kind of a nuisance. Changing the specification does seem like a good idea, especially since specification changes are relatively cheap at this stage. I don't have strong feelings between (1) and (2). (Now I'm tempted to write myself a CBOR parser to better understand how to parse this thing efficiently... next time we design one of this things, can we just use ASN.1? The data model is janky, but it avoided the serialization mistakes in CBOR, and we have a robust performant implementation in BoringSSL's openssl/bytestring.h header. You need ASN.1 to implement web packaging anyway because the ECDSA signatures are encoded in DER.)
,
Aug 15
Thanks! I'm going to proceed with the option 2 in c#12, that is: - Change the format spec so that enc(dec(cbor)) can be easily avoided in its impl. - Specifically, I'm going to merge https://github.com/WICG/webpackage/pull/287 or its minor mod - Snapshot b2, which is incompatible with b1 - kouhei will update impl draft in github and then publish it in IETF repo. - kouhei will communicate internal/external parties about the change - Implement multi-versioning support: - in Chrome M70, so that it will still accept b1 in addition to b2 for a while, and switch parsing mode adequately - in golang reference impl, so that it can emit b1 and b2
,
Aug 15
,
Aug 16
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0e992a784aeda624fa9fdae2d7c94608728b993b commit 0e992a784aeda624fa9fdae2d7c94608728b993b Author: Kouhei Ueno <kouhei@chromium.org> Date: Fri Aug 24 08:10:03 2018 SignedExchange: Switch to b2 format parser. Before this CL, Signed Exchange parser implementation in Chromium expected the b1 format [spec-b1]. This CL switches the parser implementation to expect the updated specification which we will snapshot as the b2 format in near future, and deprecate support for parsing the b1 format. Overview: - The magic strings and version specifiers are changed from b1 -> b2. - RequestUrl was a part of cbor_headers in b1, but is now located in the prologue section. - As a result, SignedExchangePrologue is now split into two: {BeforeFallbackUrl, FallbackUrlAndAfter} - Signature message bytes are now encoded using custom binary format (was CBOR in b1) - cbor_headers no longer go through enc(dec(original_bytes)), fixes crbug.com/863499 Note: fallbackUrl redirect on parse fail ( crbug.com/874323 ) is to be addressed in separate CL. [spec-b1] https://tools.ietf.org/html/draft-yasskin-httpbis-origin-signed-exchanges-impl-01 Bug: 803774, 863499 , 876968 Change-Id: Ib172411e075472dcaae21af9c7460af5b5cf4e52 Reviewed-on: https://chromium-review.googlesource.com/1183053 Commit-Queue: Kouhei Ueno <kouhei@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Kent Tamura <tkent@chromium.org> Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#585754} [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_cert_fetcher_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_certificate_chain.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_certificate_chain_fuzzer.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_certificate_chain_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_consts.h [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_envelope.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_envelope.h [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_envelope_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_handler.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_handler.h [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_handler_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_prologue.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_prologue.h [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_prologue_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_request_handler_browsertest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_signature_header_field.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_signature_header_field_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_signature_verifier.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/browser/web_package/signed_exchange_signature_verifier_unittest.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/generate-test-sxgs.sh [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.com_invalid_test.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.com_invalid_test.sxg.mock-http-headers [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.org_hello.txt.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.org_noext_test.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.org_test.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/data/sxg/test.example.org_test.sxg.mock-http-headers [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/content/test/fuzzer/signed_exchange_envelope_fuzzer.cc [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/fallback-to-another-sxg.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/generate-test-sxgs.sh [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/sxg-cert-not-found.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/sxg-invalid-validity-url.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/sxg-location-origin-trial.php [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/WebKit/LayoutTests/http/tests/loading/sxg/resources/sxg-location.sxg [modify] https://crrev.com/0e992a784aeda624fa9fdae2d7c94608728b993b/third_party/blink/tools/apache_config/mime.types
,
Aug 24
|
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by kinuko@chromium.org
, Jul 19Labels: -Pri-3 Pri-2
Status: Available (was: Untriaged)