Commit Graph

92 Commits

Author SHA1 Message Date
David Benjamin
8803c0589d Properly advance the CBS when parsing BER structures.
CBS_asn1_ber_to_der was a little cumbersome to use. While it, in theory,
allowed callers to consistently advance past the element, no caller
actually did so consistently. Instead they would advance if conversion
happened, and not if it was already DER. For the PKCS7_* functions, this
was even caller-exposed.

Change-Id: I658d265df899bace9ba6616cb465f19c9e6c3534
Reviewed-on: https://boringssl-review.googlesource.com/29304
Reviewed-by: Adam Langley <agl@google.com>
2018-06-26 07:23:10 +00:00
David Benjamin
b06f92da7b Add new character encoding functions.
These will be used for the PKCS#12 code and to replace some of the
crypto/asn1 logic. So far they support the ones implemented by
crypto/asn1, which are Latin-1, UCS-2 (ASN.1 BMPStrings can't go beyond
the BMP), UTF-32 (ASN.1 UniversalString) and UTF-8.

Change-Id: I3d5c0d964cc6f97c3a0a1e352c9dd7d8cc0d87f2
Reviewed-on: https://boringssl-review.googlesource.com/28324
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2018-05-11 21:55:26 +00:00
Adam Langley
d096c06b34 bytestring: document that |CBS_get_optional_asn1| can have a NULL output.
On the other hand, the type-specific
|CBS_get_optional_asn1_octet_string| must have a valid pointer and we
should check this in the “present” case or there could be a lucking
crash in some user waiting for an expected value to be missing.

Change-Id: Ida40e069ac7f0e50967e3f6c6b3fc01e49bd8894
Reviewed-on: https://boringssl-review.googlesource.com/26564
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-03-19 20:22:25 +00:00
Adam Langley
c61b577197 Add some more utility functions to bytestring.
Change-Id: I7932258890b0b2226ff6841af45926e1b11979ba
Reviewed-on: https://boringssl-review.googlesource.com/24844
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-01-25 23:51:36 +00:00
David Benjamin
92e332501a Add a function for encoding SET OF.
The Chromium certificate verifier ends up encoding a SET OF when
canonicalizing X.509 names. Requiring the caller canonicalize a SET OF
is complicated enough that we should probably sort it for folks. (We
really need to get this name canonicalization insanity out of X.509...)

This would remove the extra level of indirection in Chromium
net/cert/internal/verify_name_match.cc CBB usage.

Note this is not quite the same order as SET, but SET is kind of
useless. Since it's encoding heterogeneous values, it is reasonable to
require the caller just encode them in the correct order. In fact, a DER
SET is just SEQUENCE with a post-processing step on the definition to
fix the ordering of the fields. (Unless the SET contains an untagged
CHOICE, in which case the ordering is weird, but SETs are not really
used in the real world, much less SETs with untagged CHOICEs.)

Bug: 11
Change-Id: I51e7938a81529243e7514360f867330359ae4f2c
Reviewed-on: https://boringssl-review.googlesource.com/24444
Reviewed-by: Adam Langley <agl@google.com>
2018-01-05 23:39:02 +00:00
David Benjamin
d9f49974e3 Support high tag numbers in CBS/CBB.
This is a reland of https://boringssl-review.googlesource.com/2330. I
believe I've now cleared the fallout.

Android's attestion format uses some ludicrously large tag numbers:
https://developer.android.com/training/articles/security-key-attestation.html#certificate_schema

Add support for these in CBS/CBB. The public API does not change for
callers who were using the CBS_ASN1_* constants, but it is no longer the
case that tag representations match their DER encodings for small tag
numbers. When passing tags into CBS/CBB, use CBS_ASN1_* constants. When
working with DER byte arrays (most commonly test vectors), use the
numbers themselves.

Bug: 214
Update-Note: The in-memory representation of CBS/CBB tags changes.
   Additionally, we now support tag numbers above 30. I believe I've now
   actually cleared the fallout of the former. There is one test in
   Chromium and the same test in the internal repository that needs
   fixing.

Change-Id: I49b9d30df01f023c646d31156360ff69c91626a3
Reviewed-on: https://boringssl-review.googlesource.com/24404
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2018-01-03 22:28:32 +00:00
David Benjamin
2fc4f362cd Revert "Support high tag numbers in CBS/CBB."
This reverts commit 66801feb17. This
turned out to break a lot more than expected. Hopefully we can reland it
soon, but we need to fix up some consumers first.

Note due to work that went in later, this is not a trivial revert and
should be re-reviewed.

Change-Id: I6474b67cce9a8aa03f722f37ad45914b76466bea
Reviewed-on: https://boringssl-review.googlesource.com/23644
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-30 21:57:17 +00:00
David Benjamin
095b6c9baa Also add a decoupled OBJ_obj2txt.
We need it in both directions. Also I missed that in OBJ_obj2txt we
allowed uint64_t components, but in my new OBJ_txt2obj we only allowed
uint32_t. For consistency, upgrade that to uint64_t.

Bug: chromium:706445
Change-Id: I38cfeea8ff64b9acf7998e552727c6c3b2cc600f
Reviewed-on: https://boringssl-review.googlesource.com/23544
Commit-Queue: Steven Valdez <svaldez@google.com>
Reviewed-by: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-30 18:21:48 +00:00
David Benjamin
47b8f00fdc Reimplement OBJ_txt2obj and add a lower-level function.
OBJ_txt2obj is currently implemented using BIGNUMs which is absurd. It
also depends on the giant OID table, which is undesirable. Write a new
one and expose the low-level function so Chromium can use it without the
OID table.

Bug: chromium:706445
Change-Id: I61ff750a914194f8776cb8d81ba5d3eb5eaa3c3d
Reviewed-on: https://boringssl-review.googlesource.com/23364
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
2017-11-27 21:29:00 +00:00
David Benjamin
66801feb17 Support high tag numbers in CBS/CBB.
Android's attestion format uses some ludicrously large tag numbers:
https://developer.android.com/training/articles/security-key-attestation.html#certificate_schema

Add support for these in CBS/CBB. The public API does not change for
callers who were using the CBS_ASN1_* constants, but it is no longer the
case that tag representations match their DER encodings for small tag
numbers.

Chromium needs https://chromium-review.googlesource.com/#/c/chromium/src/+/783254,
but otherwise I don't expect this to break things.

Bug: 214
Change-Id: I9b5dc27ae3ea020e9edaabec4d665fd73da7d31e
Reviewed-on: https://boringssl-review.googlesource.com/23304
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-11-22 22:34:05 +00:00
David Benjamin
808f832917 Run the comment converter on libcrypto.
crypto/{asn1,x509,x509v3,pem} were skipped as they are still OpenSSL
style.

Change-Id: I3cd9a60e1cb483a981aca325041f3fbce294247c
Reviewed-on: https://boringssl-review.googlesource.com/19504
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-08-18 21:49:04 +00:00
David Benjamin
2c45fa0b90 Convert bytestring_test to GTest.
This also fixes TestGetUint to actually test CBS_get_last_u8's behavior.
Right now it can't distinguish CBS_get_last_u8 and CBS_get_u8.

BUG=129

Change-Id: Ie431bb1a828f1c6877938ba7e75c82305b54cf13
Reviewed-on: https://boringssl-review.googlesource.com/15007
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-04-17 14:19:47 +00:00
David Benjamin
76dd18008c Convert base64_test to GTest.
BUG=129

Change-Id: If91d97ea653177d55d5c703f091366ddce24da60
Reviewed-on: https://boringssl-review.googlesource.com/15006
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-04-17 14:14:17 +00:00
Victor Vasiliev
438229a8d7 Correct a typo in ASN.1 type name.
Change-Id: I5879d78ce22163c66bf8e4c05f3a7d1b6dbbd9d9
Reviewed-on: https://boringssl-review.googlesource.com/14304
Reviewed-by: Steven Valdez <svaldez@google.com>
Commit-Queue: Steven Valdez <svaldez@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2017-03-16 21:57:58 +00:00
David Benjamin
17cf2cb1d2 Work around language and compiler bug in memcpy, etc.
Most C standard library functions are undefined if passed NULL, even
when the corresponding length is zero. This gives them (and, in turn,
all functions which call them) surprising behavior on empty arrays.
Some compilers will miscompile code due to this rule. See also
https://www.imperialviolet.org/2016/06/26/nonnull.html

Add OPENSSL_memcpy, etc., wrappers which avoid this problem.

BUG=23

Change-Id: I95f42b23e92945af0e681264fffaf578e7f8465e
Reviewed-on: https://boringssl-review.googlesource.com/12928
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-12-21 20:34:47 +00:00
David Benjamin
48e2be247a Include the type in empty vector literals.
Chromium on Linux builds against libstdc++'s debug mode which makes
clang unhappy due to:

../crypto/bytestring/bytestring_test.cc:910:7: error: chosen constructor
is explicit in copy-initialization
      {},
      ^~
/usr/lib/gcc/x86_64-linux-gnu/4.8/../../../../include/c++/4.8/debug/vector:79:7:
note: constructor declared here
      vector(const _Allocator& __a = _Allocator())
      ^

I believe this was fixed here, but it's too recent:
36f540c70b

Change-Id: I2942d153e1278785c3b81294bc99b86f297cf719
Reviewed-on: https://boringssl-review.googlesource.com/12967
Reviewed-by: Adam Langley <agl@google.com>
2016-12-21 18:11:25 +00:00
David Benjamin
fcd714d52b Add CBS functions for working with BIT STRINGs.
Querying a bit in a BIT STRING is a little finicky. Add some functions
to help with this.

Change-Id: I813b9b6f2d952d61d8717b47bca1344f0ad4b7d1
Reviewed-on: https://boringssl-review.googlesource.com/12800
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-12-14 17:25:30 +00:00
David Benjamin
aac1e2dd73 Remove the remaining bssl::Main wrappers.
We've taken to writing bssl::UniquePtr in full, so it's not buying
us much.

Change-Id: Ia2689366cbb17282c8063608dddcc675518ec0ca
Reviewed-on: https://boringssl-review.googlesource.com/12628
Reviewed-by: David Benjamin <davidben@google.com>
2016-12-08 00:54:17 +00:00
David Benjamin
455919dda2 Add CBS_get_any_asn1.
We have CBS_get_asn1 / CBS_get_asn1_element, but not the "any" variants
of them. Without this, a consumer walking a DER structure must manually
CBS_skip the header, which is a little annoying.

Change-Id: I7735c37eb9e5aaad2bde8407669bce5492e1ccf6
Reviewed-on: https://boringssl-review.googlesource.com/11404
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-10-03 18:36:14 +00:00
David Benjamin
54091230cd Use C99 for size_t loops.
This was done just by grepping for 'size_t i;' and 'size_t j;'. I left
everything in crypto/x509 and friends alone.

There's some instances in gcm.c that are non-trivial and pulled into a
separate CL for ease of review.

Change-Id: I6515804e3097f7e90855f1e7610868ee87117223
Reviewed-on: https://boringssl-review.googlesource.com/10801
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-09-12 19:44:24 +00:00
David Benjamin
f0e935d7ce Fold stack-allocated types into headers.
Now that we have the extern "C++" trick, we can just embed them in the
normal headers. Move the EVP_CIPHER_CTX deleter to cipher.h and, in
doing so, take away a little bit of boilerplate in defining deleters.

Change-Id: I4a4b8d0db5274a3607914d94e76a38996bd611ec
Reviewed-on: https://boringssl-review.googlesource.com/10804
Reviewed-by: Matt Braithwaite <mab@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-09-07 21:50:05 +00:00
Matt Braithwaite
d17d74d73f Replace Scoped* heap types with bssl::UniquePtr.
Unlike the Scoped* types, bssl::UniquePtr is available to C++ users, and
offered for a large variety of types.  The 'extern "C++"' trick is used
to make the C++ bits digestible to C callers that wrap header files in
'extern "C"'.

Change-Id: Ifbca4c2997d6628e33028c7d7620c72aff0f862e
Reviewed-on: https://boringssl-review.googlesource.com/10521
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-09-01 22:22:54 +00:00
David Benjamin
1db42fb3ca Clarify CBS/CBB with respect to high tag number form.
We may need to implement high tag number form someday. CBS_get_asn1 has
an unsigned output to allow for this, but CBB_add_asn1 takes a uint8_t
(I think this might be my fault). Fix that which also fixes a
-Wconversion warning.

Simply leaving room in tag representation will still cause troubles
because the class and constructed bits overlap with bits for tag numbers
above 31. Probably the cleanest option would be to shift them to the top
3 bits of a u32 and thus not quite match the DER representation. Then
CBS_get_asn1 and CBB_add_asn1 will internally munge that into the DER
representation and consumers may continue to write things like:

   tag_number | CBS_ASN1_CONTEXT_SPECIFIC

I haven't done that here, but in preparation for that, document that
consumers need to use the values and should refrain from assuming the
correspond to DER.

Change-Id: Ibc76e51f0bc3b843e48e89adddfe2eaba4843d12
Reviewed-on: https://boringssl-review.googlesource.com/10502
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-08-26 17:48:48 +00:00
Steven Valdez
cb96654404 Adding ARRAY_SIZE macro for getting the size of constant arrays.
Change-Id: Ie60744761f5aa434a71a998f5ca98a8f8b1c25d5
Reviewed-on: https://boringssl-review.googlesource.com/10447
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-08-19 19:30:39 +00:00
Martin Kreichgauer
19d5cf86de Move remaining ScopedContext types out of scoped_types.h
Change-Id: I7d1fa964f0d9817db885cd43057a23ec46f21702
Reviewed-on: https://boringssl-review.googlesource.com/10240
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-08-11 01:15:45 +00:00
David Benjamin
22edd87755 Resolve a small handful of size_t truncation warnings.
This is very far from all of it, but I did some easy ones before I got
bored. Snapshot the progress until someone else wants to continue this.

BUG=22

Change-Id: I2609e9766d883a273e53e01a75a4b1d4700e2436
Reviewed-on: https://boringssl-review.googlesource.com/9132
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-08-05 19:12:31 +00:00
David Benjamin
4ff41f614c Check for overflow in CBB_add_u24.
All other CBB_add_u<N> functions take a narrowed type, but not every
uint32_t may fit in a u24. Check for this rather than silently truncate.

Change-Id: I23879ad0f4d2934f257e39e795cf93c6e3e878bf
Reviewed-on: https://boringssl-review.googlesource.com/8940
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-07-26 15:19:41 +00:00
David Benjamin
93a034a7d7 CBBs are in an undefined state after an operation failed.
Our CBB patterns do not make it safe to use a CBB after any operation
failed. Suppose one does:

  int add_to_cbb(CBB *cbb) {
    CBB child;
    return CBB_add_u8(cbb, 1) &&
           CBB_add_u8_length_prefixed(cbb, &child) &&
           CBB_add_u8(&child, 2) &&
           /* Flush |cbb| before |child| goes out of scoped. */
           CBB_flush(cbb);
  }

If one of the earlier operations fails, any attempt to use |cbb| (except
CBB_cleanup) would hit a memory error. Doing this would be a bug anyway,
since the CBB would be in an undefined state anyway (wrote only half my
object), but the memory error is bad manners.

Officially document that using a CBB after failure is illegal and, to
avoid the memory error, set a poison bit on the cbb_buffer_st to prevent
all future operations. In theory we could make failure +
CBB_discard_child work, but this is not very useful and would require a
more complex CBB pattern.

Change-Id: I4303ee1c326785849ce12b5f7aa8bbde6b95d2ec
Reviewed-on: https://boringssl-review.googlesource.com/8840
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
2016-07-19 20:51:51 +00:00
Adam Langley
10f97f3bfc Revert "Move C++ helpers into |bssl| namespace."
This reverts commit 09feb0f3d9.

(In order to make WebRTC happy this also needs to be reverted.)
2016-07-12 08:09:33 -07:00
Adam Langley
d2b5af56cf Revert scoped_types.h change.
This reverts commits:
8d79ed6740
19fdcb5234
8d79ed6740

Because WebRTC (at least) includes our headers in an extern "C" block,
which precludes having any C++ in them.

Change-Id: Ia849f43795a40034cbd45b22ea680b51aab28b2d
2016-07-12 08:05:38 -07:00
Adam Langley
8c3c3135a2 Remove scoped_types.h.
This change scatters the contents of the two scoped_types.h files into
the headers for each of the areas of the code. The types are now in the
|bssl| namespace.

Change-Id: I802b8de68fba4786b6a0ac1bacd11d81d5842423
Reviewed-on: https://boringssl-review.googlesource.com/8731
Reviewed-by: Adam Langley <agl@google.com>
2016-07-11 23:08:27 +00:00
Adam Langley
09feb0f3d9 Move C++ helpers into |bssl| namespace.
We currently have the situation where the |tool| and |bssl_shim| code
includes scoped_types.h from crypto/test and ssl/test. That's weird and
shouldn't happen. Also, our C++ consumers might quite like to have
access to the scoped types.

Thus this change moves some of the template code to base.h and puts it
all in a |bssl| namespace to prepare for scattering these types into
their respective headers. In order that all the existing test code be
able to access these types, it's all moved into the same namespace.

Change-Id: I3207e29474dc5fcc344ace43119df26dae04eabb
Reviewed-on: https://boringssl-review.googlesource.com/8730
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-11 23:04:52 +00:00
David Benjamin
bb076e334c Add CBB_add_u32.
It was missing. Writing NewSessionTicket will need it.

Change-Id: I39de237894f2e8356bd6861da2b8a4d805dcd2d6
Reviewed-on: https://boringssl-review.googlesource.com/8439
Reviewed-by: Adam Langley <agl@google.com>
2016-06-27 20:12:54 +00:00
David Benjamin
a7810c12e9 Make tls_open_record always in-place.
The business with ssl_record_prefix_len is rather a hassle. Instead, have
tls_open_record always decrypt in-place and give back a CBS to where the body
is.

This way the caller doesn't need to do an extra check all to avoid creating an
invalid pointer and underflow in subtraction.

Change-Id: I4e12b25a760870d8f8a503673ab00a2d774fc9ee
Reviewed-on: https://boringssl-review.googlesource.com/8173
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-08 18:39:07 +00:00
Nick Harper
6f621bd8f7 Merge documentation from chromium's net/der into cbs.c
Change-Id: Icfd959a168e3fce423b10dd0dcb1312ec03f0623
Reviewed-on: https://boringssl-review.googlesource.com/7800
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-28 19:39:40 +00:00
Matt Mueller
897be6afe3 Add CBS_ASN1_UTF8STRING define.
Change-Id: I34384feb46c15c4f443f506d724ad500a4cf0f36
Reviewed-on: https://boringssl-review.googlesource.com/7525
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-23 19:29:49 +00:00
David Benjamin
bd42603943 Add a convenience function for i2d compatibility wrappers.
An i2d compatibility function is rather long, so add CBB_finish_i2d for
part of it. It takes a CBB as input so only a 'marshal' function is
needed, rather than a 'to_bytes' one.

Also replace the *inp d2i update pattern with a slightly shorter one.

Change-Id: Ibb41059c9532f6a8ce33460890cc1afe26adc97c
Reviewed-on: https://boringssl-review.googlesource.com/6868
Reviewed-by: Adam Langley <agl@google.com>
2016-02-16 19:40:53 +00:00
David Benjamin
375124b162 Parse BER for PKCS#12 more accurately.
CBS_asn1_ber_to_der currently uses heuristics because implicitly-tagged
constructed strings in BER are ambiguous with implicitly-tagged sequences. It's
not possible to convert BER to DER without knowing the schema.

Fortunately, implicitly tagged strings don't appear often so instead split the
job up: CBS_asn1_ber_to_der fixes indefinite-length elements and constructed
strings it can see. Implicitly-tagged strings it leaves uncoverted, but they
will only nest one level down (because BER kindly allows one to nest
constructed strings arbitrarily!).

CBS_get_asn1_implicit_string then performs the final concatenation at parse
time. This isn't much more complex and lets us parse BER more accurately and
also reject a number of mis-encoded values (e.g. constructed INTEGERs are not a
thing) we'd previously let through. The downside is the post-conversion parsing
code must be aware of this limitation of CBS_asn1_ber_to_der. Fortunately,
there's only one implicitly-tagged string in our PKCS#12 code.

(In the category of things that really really don't matter, but I had spare
cycles and the old BER converter is weird.)

Change-Id: Iebdd13b08559fa158b308ef83a5bb07bfdf80ae8
Reviewed-on: https://boringssl-review.googlesource.com/7052
Reviewed-by: Adam Langley <agl@google.com>
2016-02-16 19:32:50 +00:00
David Benjamin
11aac10987 Fix theoretical memory leak on malloc error in CBS_asn1_ber_to_der.
On failure, CBB_finish doesn't call CBB_cleanup. Also chain more of the ||s
together now that CBB_cleanup after failed CBB_init is legal.

(I don't think this is actually reachable because the CBB is guaranteed to be
flushed by this point.)

Change-Id: Ib16a0a185f15e13675ac2550c5e8e0926ceb7957
Reviewed-on: https://boringssl-review.googlesource.com/7051
Reviewed-by: Adam Langley <agl@google.com>
2016-02-04 17:23:28 +00:00
David Benjamin
2a0b391ac9 Rewrite ssl3_send_server_key_exchange to use CBB.
There is some messiness around saving and restoring the CBB, but this is
still significantly clearer.

Note that the BUF_MEM_grow line is gone in favor of a fixed CBB like the
other functions ported thus far. This line was never necessary as
init_buf is initialized to 16k and none of our key exchanges get that
large. (The largest one can get is DHE_RSA. Even so, it'd take a roughly
30k-bit DH group with a 30k-bit RSA key.)

Having such limits and tight assumptions on init_buf's initial size is
poor (but on par for the old code which usually just blindly assumed the
message would not get too large) and the size of the certificate chain
is much less obviously bounded, so those BUF_MEM_grows can't easily go.

My current plan is convert everything but those which legitimately need
BUF_MEM_grow to CBB, then atomically convert the rest, remove init_buf,
and switch everything to non-fixed CBBs. This will hopefully also
simplify async resumption. In the meantime, having a story for
resumption means the future atomic change is smaller and, more
importantly, relieves some complexity budget in the ServerKeyExchange
code for adding Curve25519.

Change-Id: I1de6af9856caaed353453d92a502ba461a938fbd
Reviewed-on: https://boringssl-review.googlesource.com/6770
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 17:23:58 +00:00
David Benjamin
4cc671cbf4 Add CBB_reserve and CBB_did_write.
These will be needed when we start writing variable-length things to a
CBB.

Change-Id: Ie7b9b140f5f875b43adedc8203ce9d3f4068dfea
Reviewed-on: https://boringssl-review.googlesource.com/6764
Reviewed-by: Adam Langley <agl@google.com>
2015-12-22 00:23:52 +00:00
David Benjamin
a01deee96b Make CBB_len relative to its argument.
Rather than the length of the top-level CBB, which is kind of odd when ASN.1
length prefixes are not yet determined, return the number of bytes written to
the CBB so far. This can be computed without increasing the size of CBB at all.
Have offset and pending_*.

This means functions which take in a CBB as argument will not be sensitive to
whether the CBB is a top-level or child CBB. The extensions logic had to be
careful to only ever compare differences of lengths, which was awkward.

The reversal will also allow for the following pattern in the future, once
CBB_add_space is split into, say, CBB_reserve and CBB_did_write and we add a
CBB_data:

  uint8_t *signature;
  size_t signature_len = 0;
  if (!CBB_add_asn1(out, &cert, CBB_ASN1_SEQUENCE) ||
      /* Emit the TBSCertificate. */
      !CBB_add_asn1(&cert, &tbs_cert, CBS_ASN1_SEQUENCE) ||
      !CBB_add_tbs_cert_stuff(&tbs_cert, stuff) ||
      !CBB_flush(&cert) ||
      /* Feed it into md_ctx. */
      !EVP_DigestSignInit(&md_ctx, NULL, EVP_sha256(), NULL, pkey) ||
      !EVP_DigestSignUpdate(&md_ctx, CBB_data(&cert), CBB_len(&cert)) ||
      /* Emit the signature algorithm. */
      !CBB_add_asn1(&cert, &sig_alg, CBS_ASN1_SEQUENCE) ||
      !CBB_add_sigalg_stuff(&sig_alg, other_stuff) ||
      /* Emit the signature. */
      !EVP_DigestSignFinal(&md_ctx, NULL, &signature_len) ||
      !CBB_reserve(&cert, &signature, signature_len) ||
      !EVP_DigestSignFinal(&md_ctx, signature, &signature_len) ||
      !CBB_did_write(&cert, signature_len)) {
    goto err;
  }

(Were TBSCertificate not the first field, we'd still have to sample
CBB_len(&cert), but at least that's reasonable straight-forward. The
alternative would be if CBB_data and CBB_len somehow worked on
recently-invalidated CBBs, but that would go wrong once the invalidated CBB's
parent flushed and possibly shifts everything.)

And similar for signing ServerKeyExchange.

Change-Id: I7761e492ae472d7632875b5666b6088970261b14
Reviewed-on: https://boringssl-review.googlesource.com/6681
Reviewed-by: Adam Langley <agl@google.com>
2015-12-16 21:16:12 +00:00
David Benjamin
2077cf9152 Use UINT64_C instead of OPENSSL_U64.
stdint.h already has macros for this. The spec says that, in C++,
__STDC_CONSTANT_MACROS is needed, so define it for bytestring_test.cc.
Chromium seems to use these macros without trouble, so I'm assuming we
can rely on them.

Change-Id: I56d178689b44d22c6379911bbb93d3b01dd832a3
Reviewed-on: https://boringssl-review.googlesource.com/6510
Reviewed-by: Adam Langley <agl@google.com>
2015-11-16 23:18:00 +00:00
Adam Langley
f9c77dedfa Drop CBB allocation failure test.
To no great surprise, ASAN didn't like this test and I suspect that
Chromium, with its crashing allocator, won't like it either. Oh well.

Change-Id: I235dbb965dbba186f8f37d7df45f8eac9addc7eb
Reviewed-on: https://boringssl-review.googlesource.com/6496
Reviewed-by: Adam Langley <agl@google.com>
2015-11-16 22:25:59 +00:00
Adam Langley
a33915d690 Have |CBB_init| zero the |CBB| before any possible failures.
People expect to do:

CBB foo;

if (!CBB_init(&foo, 100) ||
    …
    …) {
  CBB_cleanup(&foo);
  return 0;
}

However, currently, if the allocation of |initial_capacity| fails in
|CBB_init| then |CBB_cleanup| will operate on uninitialised values. This
change makes the above pattern safe.

Change-Id: I3e002fda8f0a3ac18650b504e7e84a842d4165ca
Reviewed-on: https://boringssl-review.googlesource.com/6495
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
2015-11-16 21:59:46 +00:00
David Benjamin
ef14b2d86e Remove stl_compat.h.
Chromium's toolchains may now assume C++11 library support, so we may freely
use C++11 features. (Chromium's still in the process of deciding what to allow,
but we use Google's style guide directly, toolchain limitations aside.)

Change-Id: I1c7feb92b7f5f51d9091a4c686649fb574ac138d
Reviewed-on: https://boringssl-review.googlesource.com/6465
Reviewed-by: Adam Langley <agl@google.com>
2015-11-11 22:19:36 +00:00
Matt Braithwaite
fa9eb568b0 Correct the spelling of "primitive".
Spotted by Matt Smart.

Change-Id: Id9c61ba6a293ddc52b2e2c93c427860765848c6d
Reviewed-on: https://boringssl-review.googlesource.com/6430
Reviewed-by: Adam Langley <agl@google.com>
2015-11-03 21:47:19 +00:00
David Benjamin
301afaf223 Add a run_tests target to run all tests.
It's very annoying having to remember the right incant every time I want
to switch around between my build, build-release, build-asan, etc.,
output directories.

Unfortunately, this target is pretty unfriendly without CMake 3.2+ (and
Ninja 1.5+). This combination gives a USES_TERMINAL flag to
add_custom_target which uses Ninja's "console" pool, otherwise the
output buffering gets in the way. Ubuntu LTS is still on an older CMake,
so do a version check in the meantime.

CMake also has its own test mechanism (CTest), but this doesn't use it.
It seems to prefer knowing what all the tests are and then tries to do
its own output management and parallelizing and such. We already have
our own runners. all_tests.go could actually be converted tidily, but
generate_build_files.py also needs to read it, and runner.go has very
specific needs.

Naming the target ninja -C build test would be nice, but CTest squats
that name and CMake grumps when you use a reserved name, so I've gone
with run_tests.

Change-Id: Ibd20ebd50febe1b4e91bb19921f3bbbd9fbcf66c
Reviewed-on: https://boringssl-review.googlesource.com/6270
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-26 20:33:44 +00:00
David Benjamin
e8d53508ca Convert ssl3_send_client_hello to CBB.
Start converting the ones we can right now. Some of the messier ones
resize init_buf rather than assume the initial size is sufficient, so
those will probably wait until init_buf is gone and the handshake's
undergone some more invasive surgery. The async ones will also require
some thought. But some can be incrementally converted now.

BUG=468889

Change-Id: I0bc22e4dca37d9d671a488c42eba864c51933638
Reviewed-on: https://boringssl-review.googlesource.com/6190
Reviewed-by: Adam Langley <alangley@gmail.com>
2015-10-20 17:56:19 +00:00
David Benjamin
dbb0321014 Clarify that only top-level CBBs get CBB_cleanup.
Also add an assert to that effect.

Change-Id: I1bd0571e3889f1cba968fd99041121ac42ee9e89
Reviewed-on: https://boringssl-review.googlesource.com/5990
Reviewed-by: Adam Langley <agl@google.com>
2015-09-28 22:40:01 +00:00