Commit Graph

163 Commits

Author SHA1 Message Date
David Benjamin
a1bbacac51 Record kCRLTestRoot's private key in the source.
At some point, we'll forget to look in the commit message.

Change-Id: I3153aab679209f4f11f56cf3f883c4c74a17af1d
Reviewed-on: https://boringssl-review.googlesource.com/11800
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
2016-10-25 23:24:21 +00:00
David Benjamin
a7f56f0bfb Use scopers a little more.
Change-Id: I51e5a7dac3ceffc41d3a7a57157a11258e65bc42
Reviewed-on: https://boringssl-review.googlesource.com/11721
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-10-24 20:10:18 +00:00
David Benjamin
be941d4659 Correctly find all critical CRL extensions.
Unhandled critical CRL extensions were not detected if they appeared
after the handled ones.  (Upstream GitHub issue 1757).  Thanks to John
Chuah for reporting this.

(Imported from upstream's 3ade92e785bb3777c92332f88e23f6ce906ee260.)

This additionally adds a regression test for this issue, generated with
der-ascii.  The signatures on the CRLs were repaired per notes in
https://github.com/google/der-ascii/blob/master/samples/certificates.md

Change-Id: I74a77f92710e6ef7f46dcde5cb6ae9350084ddcb
Reviewed-on: https://boringssl-review.googlesource.com/11720
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-10-24 20:09:28 +00:00
David Benjamin
b1133e9565 Fix up macros.
Macros need a healthy dose of parentheses to avoid expression-level
misparses. Most of this comes from the clang-tidy CL here:
https://android-review.googlesource.com/c/235696/

Also switch most of the macros to use do { ... } while (0) to avoid all
the excessive comma operators and statement-level misparses.

Change-Id: I4c2ee51e347d2aa8c74a2d82de63838b03bbb0f9
Reviewed-on: https://boringssl-review.googlesource.com/11660
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-10-18 18:28:23 +00:00
David Benjamin
54afdab584 Fix X509_CRL construction under malloc failures.
If asn1_item_ex_combine_new fails in one of the ASN1_template_new calls
just before the ASN1_OP_NEW_POST call, ASN1_item_ex_free will free the
temporary object which ultimately calls ASN1_OP_FREE_POST. This means
that ASN1_OP_FREE_POST needs to account for zero-initialized objects.

Change-Id: I56fb63bd5c015d9dfe3961606449bc6f5b1259e3
Reviewed-on: https://boringssl-review.googlesource.com/11403
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-09-30 19:13:36 +00:00
David Benjamin
daf4a05bf4 Avoid using empty initializer lists.
One of Chromium's toolchains can't handle this for some reason. See also
empty_crls and empty in TestVerify.

Change-Id: I5e6a849f3042288da2e406882ae5cfec249a86ae
Reviewed-on: https://boringssl-review.googlesource.com/11340
Reviewed-by: Steven Valdez <svaldez@google.com>
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-27 18:11:29 +00:00
Steven Valdez
f9f312af61 Add some sanity checks when checking CRL scores and tests
Note: this was accidentally omitted from OpenSSL 1.0.2 branch.
Without this fix any attempt to use CRLs will crash.

CVE-2016-7052

(Imported from upstream's 6e629b5be45face20b4ca71c4fcbfed78b864a2e)

Test CRL Root Key:

-----BEGIN RSA PRIVATE KEY-----
MIIEpAIBAAKCAQEAo16WiLWZuaymsD8n5SKPmxV1y6jjgr3BS/dUBpbrzd1aeFzN
lI8l2jfAnzUyp+I21RQ+nh/MhqjGElkTtK9xMn1Y+S9GMRh+5R/Du0iCb1tCZIPY
07Tgrb0KMNWe0v2QKVVruuYSgxIWodBfxlKO64Z8AJ5IbnWpuRqO6rctN9qUoMlT
IAB6dL4G0tDJ/PGFWOJYwOMEIX54bly2wgyYJVBKiRRt4f7n8H922qmvPNA9idmX
9G1VAtgV6x97XXi7ULORIQvn9lVQF6nTYDBJhyuPB+mLThbLP2o9orxGx7aCtnnB
ZUIxUvHNOI0FaSaZH7Fi0xsZ/GkG2HZe7ImPJwIDAQABAoIBAQCJF9MTHfHGkk+/
DwCXlA0Wg0e6hBuHl10iNobYkMWIl/xXjOknhYiqOqb181py76472SVC5ERprC+r
Lf0PXzqKuA117mnkwT2bYLCL9Skf8WEhoFLQNbVlloF6wYjqXcYgKYKh8HgQbZl4
aLg2YQl2NADTNABsUWj/4H2WEelsODVviqfFs725lFg9KHDI8zxAZXLzDt/M9uVL
GxJiX12tr0AwaeAFZ1oPM/y+LznM3N3+Ht3jHHw3jZ/u8Z1RdAmdpu3bZ6tbwGBr
9edsH5rKkm9aBvMrY7eX5VHqaqyRNFyG152ZOJh4XiiFG7EmgTPCpaHo50Y018Re
grVtk+FBAoGBANY3lY+V8ZOwMxSHes+kTnoimHO5Ob7nxrOC71i27x+4HHsYUeAr
/zOOghiDIn+oNkuiX5CIOWZKx159Bp65CPpCbTb/fh+HYnSgXFgCw7XptycO7LXM
5GwR5jSfpfzBFdYxjxoUzDMFBwTEYRTm0HkUHkH+s+ajjw5wqqbcGLcfAoGBAMM8
DKW6Tb66xsf708f0jonAjKYTLZ+WOcwsBEWSFHoY8dUjvW5gqx5acHTEsc5ZTeh4
BCFLa+Mn9cuJWVJNs09k7Xb2PNl92HQ4GN2vbdkJhExbkT6oLDHg1hVD0w8KLfz1
lTAW6pS+6CdOHMEJpvqx89EgU/1GgIQ1fXYczE75AoGAKeJoXdDFkUjsU+FBhAPu
TDcjc80Nm2QaF9NMFR5/lsYa236f06MGnQAKM9zADBHJu/Qdl1brUjLg1HrBppsr
RDNkw1IlSOjhuUf5hkPUHGd8Jijm440SRIcjabqla8wdBupdvo2+d2NOQgJbsQiI
ToQ+fkzcxAXK3Nnuo/1436UCgYBjLH7UNOZHS8OsVM0I1r8NVKVdu4JCfeJQR8/H
s2P5ffBir+wLRMnH+nMDreMQiibcPxMCArkERAlE4jlgaJ38Z62E76KLbLTmnJRt
EC9Bv+bXjvAiHvWMRMUbOj/ddPNVez7Uld+FvdBaHwDWQlvzHzBWfBCOKSEhh7Z6
qDhUqQKBgQDPMDx2i5rfmQp3imV9xUcCkIRsyYQVf8Eo7NV07IdUy/otmksgn4Zt
Lbf3v2dvxOpTNTONWjp2c+iUQo8QxJCZr5Sfb21oQ9Ktcrmc/CY7LeBVDibXwxdM
vRG8kBzvslFWh7REzC3u06GSVhyKDfW93kN2cKVwGoahRlhj7oHuZQ==
-----END RSA PRIVATE KEY-----

Change-Id: Icc58811c78d4682591f5bb460cdd219bd41566d8
Reviewed-on: https://boringssl-review.googlesource.com/11246
Reviewed-by: Steven Valdez <svaldez@google.com>
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-26 18:06:52 +00:00
David Benjamin
16279bc559 Fix up x509_vpm.c comment.
We used upstream's reformat script, but they had stuck hyphens
everywhere to tell indent to leave a comment alone. Fix this one since
it was especially hard to read.

Change-Id: I9f43bd57dbcf66b79b775ad10ee67867d815ed33
Reviewed-on: https://boringssl-review.googlesource.com/11243
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-09-23 18:39:11 +00:00
David Benjamin
1ccfb4e32d Shush a MinGW warning in crypto/x509.
The MinGW setup on Android already defines this stat macro.

Change-Id: Ia8e89195c06ec01d4b5a2fa7357fb8d2d500aa06
Reviewed-on: https://boringssl-review.googlesource.com/11124
Reviewed-by: Kenny Root <kroot@google.com>
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-20 22:26:27 +00:00
Alessandro Ghedini
1fc7e9ccd2 Remove trailing ';' from macros
For consistency and to avoid a pedantic GCC warning (even though it's
mostly old legacy code).

Change-Id: Iea63eb0a82ff52914adc33b83e48450f4f6a49ef
Reviewed-on: https://boringssl-review.googlesource.com/11021
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:17:26 +00:00
Steven Valdez
934b57e8c9 Fix a few leaks in X509_REQ_to_X509.
(Imported from upstream's a404656a8b40d9f1172e5e330f7e2d9d87cabab8)

Change-Id: I4ddebfbaeab433bae7c1393a8258d786801bb633
Reviewed-on: https://boringssl-review.googlesource.com/10920
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-09 20:17:16 +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
96a16cd10e Finish aligning up_ref functions with OpenSSL 1.1.0.
All external callers should be resolved now.

BUG=89

Change-Id: I6055450e8202c59cca49e4a824be3ec11c32a15a
Reviewed-on: https://boringssl-review.googlesource.com/10285
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 16:51:52 +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
5a91503826 Add various 1.1.0 accessors.
This gets cURL building against both BoringSSL as it is and BoringSSL
with OPENSSL_VERSION_NUMBER set to 1.1.0.

BUG=91

Change-Id: I5be73b84df701fe76f3055b1239ae4704a931082
Reviewed-on: https://boringssl-review.googlesource.com/10180
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
2016-08-10 16:52:15 +00:00
David Benjamin
e76cdde77d Use newest CRL.
If two CRLs are equivalent then use the one with a later lastUpdate field:
this will result in the newest CRL available being used.

(Imported from upstream's 325da8231c8d441e6bb7f15d1a5a23ff63c842e5 and
3dc160e9be6dcaeec9345fbb61b1c427d7026103.)

Change-Id: I8c722663b979dfe08728d091697d8b8204dc265c
Reviewed-on: https://boringssl-review.googlesource.com/8947
Commit-Queue: David Benjamin <davidben@google.com>
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-02 17:45:15 +00:00
David Benjamin
69e0a457a1 Remove OPENSSL_ALLOW_PROXY_CERTS.
One less random environment variable for us to be sensitive to. (We
should probably unwind all this proxy cert stuff. I don't believe they
are ever enabled.)

Change-Id: I74993178679ea49e60c81d8416e502cbebf02ec9
Reviewed-on: https://boringssl-review.googlesource.com/8948
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-07-26 22:49:52 +00:00
David Benjamin
64ac925601 Fix ASN1_STRING_to_UTF8 could not convert NumericString
tag2nbyte had -1 at 18th position, but underlying ASN1_mbstring_copy
supports NumericString. tag2nbyte is also used in do_print_ex which will
not be broken by setting 1 at 18th position of tag2nbyte

(Imported from upstream's bd598cc405e981de259a07558e600b5a9ef64bd6.)

Change-Id: Ie063afcaac8a7d5046cdb385059b991b92cd6659
Reviewed-on: https://boringssl-review.googlesource.com/8946
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>
2016-07-26 22:41:42 +00:00
David Benjamin
ee2aea0d9b Fix an error path leak in int X509_ATTRIBUTE_set1_data()
(Imported from upstream's e6f65f769d87846bdc5b58ef8d2ef4074044022d.)

Change-Id: I95df13561481e98faaf8227561228c151dd344b6
Reviewed-on: https://boringssl-review.googlesource.com/8942
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 19:53:44 +00:00
David Benjamin
00d7a7cee7 Drop cached certificate signature validity flag
It seems risky in the context of cross-signed certificates when the
same certificate might have multiple potential issuers.  Also rarely
used, since chains in OpenSSL typically only employ self-signed
trust-anchors, whose self-signatures are not checked, while untrusted
certificates are generally ephemeral.

(Imported from upstream's 0e76014e584ba78ef1d6ecb4572391ef61c4fb51.)

This is in master and not 1.0.2, but having a per-certificate signature
cache when this is a function of signature and issuer seems dubious at
best. Thanks to Viktor Dukhovni for pointing this change out to me.
(And for making the original change upstream, of course.)

Change-Id: Ie692d651726f14aeba6eaab03ac918fcaedb4eeb
Reviewed-on: https://boringssl-review.googlesource.com/8880
Reviewed-by: Adam Langley <agl@google.com>
2016-07-21 17:46:15 +00:00
Adam Langley
310d3f63f3 Change |EVP_PKEY_up_ref| to return int.
Upstream have added |EVP_PKEY_up_ref|, but their version returns an int.
Having this function with a different signature like that is dangerous
so this change aligns BoringSSL with upstream. Users of this function in
Chromium and internally should already have been updated.

Change-Id: I0a7aeaf1a1ca3b0f0c635e2ee3826aa100b18157
Reviewed-on: https://boringssl-review.googlesource.com/8736
Reviewed-by: David Benjamin <davidben@google.com>
2016-07-12 17:55:41 +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
da7f0c65ef Unwind X509_LU_RETRY and fix a lot of type confusion.
(This change will be sent upstream. Since the legacy X.509 stack is just
kept around for compatibility, if they decide to fix it in a different
way, we may wish to revert this and apply their fix.)

Dating back to SSLeay, X509_LOOKUP_METHOD had this X509_LU_RETRY
machinery. But it's not documented and it appears to have never worked.

Problems with the existing logic:

- X509_LU_* is not sure whether it is a type enum (to be passed into
  X509_LOOKUP_by_*) or a return enum (to be retained by those same
  functions).

- X509_LOOKUP_by_* is not sure whether it returns 0/1 or an X509_LU_*
  value.  Looking at the functions themselves, one might think it's the
  latter, but for X509_LOOKUP_by_subject returning both 0 and
  X509_LU_FAIL. But looking at the call sites, some expect 0/1 (such as
  X509_STORE_get1_certs) while others expect an X509_LU_* enum (such as
  X509_STORE_CTX_get1_issuer). It is very fortunate that FAIL happens to
  be 0 and X509 happens to be 1.

  These functions primarily call to X509_LOOKUP_METHOD hooks. Looking
  through OpenSSL itself and code checked into Google, I found no
  evidence that any hooks have been implemented except for
  get_by_subject in by_dir.c. We take that one as definitive and observe
  it believes it returns 0/1. Notably, it returns 1 on success even if
  asked for a type other than X509_LU_X509. (X509_LU_X509 = 1. Others are
  different.) I found another piece of third-party software which corroborates
  this worldview.

- X509_STORE_get_by_subject's handling of X509_LU_RETRY (it's the j < 0
  check) is broken. It saves j into vs->current_method where it probably
  meant to save i. (This bug has existed since SSLeay.)

  It also returns j (supposedly X509_LU_RETRY) while all callers of
  X509_STORE_get_by_subject expect it to return 0/1 by checking with !
  instead of <= 0. (Note that all other codepaths return 0 and 1 so this
  function did not actually believe it returned X509_LU_* most of the
  time.)

  This, in turn, gives us a free of uninitialized pointers in
  X509_STORE_get1_certs and other functions which expect that *ret is
  filled in if X509_STORE_get_by_subject returns success. GCC 4.9 with
  optimizations from the Android NDK noticed this, which trigged this
  saga.

  (It's only reachable if any X509_LOOKUP_METHOD returned
  X509_LU_RETRY.)

- Although the code which expects X509_STORE_get_by_subject return 0/1
  does not date to SSLeay, the X509_STORE_get_by_subject call in
  X509_STORE_CTX_get1_issuer *does* (though, at the time, it was inline
  in X509_verify_cert. That code believes X509_STORE_get_by_subject
  returns an X509_LU_* enum, but it doesn't work either! It believes
  *ret is filled in on X509_LU_RETRY, thus freeing another uninitialized
  pointer (GCC noticed this too).

Since this "retry" code has clearly never worked, from SSLeay onwards,
unwind it completely rather than attempt to fix it. No
X509_LOOKUP_METHOD can possibly have depended on it.

Matching all non-broken codepaths X509_LOOKUP_by_* now returns 0/1 and
X509_STORE_get_by_subject returns 0/1. X509_LU_* is purely a type enum
with X509_LU_{REJECT,FAIL} being legacy constants to keep old code
compiling. (Upstream is recommended to remove those values altogether
for 1.1.0.)

On the off chance any get_by_* X509_LOOKUP_METHOD implementations did
not return 0/1 (I have found no evidence anywhere of this, and I believe
it wouldn't have worked anyway), the X509_LOOKUP_by_* wrapper functions
will coerce the return values back to 0/1 before passing up to the
callers which want 0/1. This both avoids the error-prone -1/0/1 calling
convention and, more importantly, avoids problems with third-party
callers which expect a X509_LU_* return code. 0/1 collide with FAIL/X509
while -1 will collide with RETRY and might confuse things.

Change-Id: I98ecf6fa7342866b9124dc6f0b422cb9ce4a1ae7
Reviewed-on: https://boringssl-review.googlesource.com/8303
Reviewed-by: Adam Langley <agl@google.com>
2016-06-16 16:24:44 +00:00
David Benjamin
e77b16ef71 Remove ASN.1 print hooks.
These functions are never instantiated. (They're a remnant of the PKCS#7 and
CMS bits.) Next time upstream touches this code, we don't have to puzzle
through the diff and import it.

Change-Id: I67c2102ae13e8e0527d858e1c63637dd442a4ffb
Reviewed-on: https://boringssl-review.googlesource.com/8242
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-14 17:38:31 +00:00
David Benjamin
d0c677cd8e Avoid illegal pointers in asn1_string_canon.
(Imported from upstream's 3892b95750b6aa5ed4328a287068f7cdfb9e55bc.)

More reasonable would have been to drop |to| altogether and act on from[len-1],
but I suppose this works.

Change-Id: I280b4991042b4d330ba034f6a631f8421ddb2643
Reviewed-on: https://boringssl-review.googlesource.com/8241
Reviewed-by: Adam Langley <agl@google.com>
2016-06-13 21:57:05 +00:00
David Benjamin
8f1e113a73 Ensure verify error is set when X509_verify_cert() fails.
Set ctx->error = X509_V_ERR_OUT_OF_MEM when verification cannot
continue due to malloc failure.  Similarly for issuer lookup failures
and caller errors (bad parameters or invalid state).

Also, when X509_verify_cert() returns <= 0 make sure that the
verification status does not remain X509_V_OK, as a last resort set
it it to X509_V_ERR_UNSPECIFIED, just in case some code path returns
an error without setting an appropriate value of ctx->error.

Add new and some missing error codes to X509 error -> SSL alert switch.

(Imported from upstream's 5553a12735e11bc9aa28727afe721e7236788aab.)

Change-Id: I3231a6b2e72a3914cb9316b8e90ebaee009a1c5f
Reviewed-on: https://boringssl-review.googlesource.com/8170
Reviewed-by: David Benjamin <davidben@google.com>
2016-06-09 17:29:39 +00:00
David Benjamin
29270dea85 Split unlock functions into read/write variants.
Windows SRWLOCK requires you call different functions here. Split
them up in preparation for switching Windows from CRITICAL_SECTION.

BUG=37

Change-Id: I7b5c6a98eab9ae5bb0734b805cfa1ff334918f35
Reviewed-on: https://boringssl-review.googlesource.com/8080
Reviewed-by: Adam Langley <agl@google.com>
2016-05-31 21:09:29 +00:00
David Benjamin
641f42b1a2 Make i2d_X509_AUX work if *pp = NULL.
When *pp is NULL, don't write garbage, return an unexpected pointer
or leak memory on error.

(Imported from upstream's 36c37944909496a123e2656ad1f651769a7cc72f.)

This calling convention...

Change-Id: Ic733092cfb942a3e1d3ceda6797222901ad55bef
Reviewed-on: https://boringssl-review.googlesource.com/7944
Reviewed-by: Adam Langley <agl@google.com>
2016-05-13 13:53:48 +00:00
David Benjamin
e31e0123ea Add size limit to X509_NAME structure.
This adds an explicit limit to the size of an X509_NAME structure. Some
part of OpenSSL (e.g. TLS) already effectively limit the size due to
restrictions on certificate size.

See also upstream's 65cb92f4da37a3895437f0c9940ee0bcf9f28c8a, although this is
different from upstream's. Upstream's version bounds both the X509_NAME *and*
any data after it in the immediately containing structure. While adding a bound
on all of crypto/asn1 is almost certainly a good idea (will look into that for
a follow-up), it seems bizarre and unnecessary to have X509_NAME affect its
parent.

Change-Id: Ica2136bcd1455d7c501ccc6ef2a19bc5ed042501
Reviewed-on: https://boringssl-review.googlesource.com/7846
Reviewed-by: Adam Langley <agl@google.com>
2016-05-03 16:43:52 +00:00
David Benjamin
a43fd90c5d Sync with upstream on i2d_X509_AUX.
Upstream decided to reset *pp on error and to later fix up the other i2d
functions to behave similarly. See upstream's
c5e603ee182b40ede7713c6e229c15a8f3fdb58a.

Change-Id: I01f82b578464060d0f2be5460fe4c1b969124c8e
Reviewed-on: https://boringssl-review.googlesource.com/7844
Reviewed-by: Adam Langley <agl@google.com>
2016-05-03 16:37:19 +00:00
David Benjamin
52a3bf2835 Add checks to X509_NAME_oneline()
Sanity check field lengths and sums to avoid potential overflows and reject
excessively large X509_NAME structures.

Issue reported by Guido Vranken.

(Imported from upstream's 9b08619cb45e75541809b1154c90e1a00450e537.)

Change-Id: Ib2e1e7cd086f9c3f0d689d61947f8ec3e9220049
Reviewed-on: https://boringssl-review.googlesource.com/7842
Reviewed-by: Adam Langley <agl@google.com>
2016-05-03 16:34:59 +00:00
David Benjamin
ddc69230f3 Sanity check buffer length.
Reject zero length buffers passed to X509_NAME_oneline().

Issue reported by Guido Vranken.

(Imported from upstream's 66e731ab09f2c652d0e179df3df10d069b407604.)

Tweaked slightly to use <= 0 instead of == 0 since the length is signed.

Change-Id: I5ee54d77170845e4699fda7df5e94538c8e55ed9
Reviewed-on: https://boringssl-review.googlesource.com/7841
Reviewed-by: Adam Langley <agl@google.com>
2016-05-03 16:30:41 +00:00
David Benjamin
7acd6bc07a Start assuming MSVC 2015.
BUG=43

Change-Id: I46ad1ca62b8921a03fae51f5d7bbe1c68fc0b170
Reviewed-on: https://boringssl-review.googlesource.com/7821
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-05-02 19:46:25 +00:00
David Benjamin
d18cb77864 Fix d2i_X509_AUX.
The logic to reset *pp doesn't actually work if pp is NULL. (It also doesn't
work if *pp is NULL, but that didn't work before either.) Don't bother
resetting it. This is consistent with the template-based i2d functions which do
not appear to leave *pp alone on error.

Will send this upstream.

Change-Id: I9fb5753e5d36fc1d490535720b8aa6116de69a70
Reviewed-on: https://boringssl-review.googlesource.com/7812
Reviewed-by: Adam Langley <agl@google.com>
2016-04-29 20:26:52 +00:00
Steven Valdez
b32a9151da Ensure we check i2d_X509 return val
The i2d_X509() function can return a negative value on error. Therefore
we should make sure we check it.

Issue reported by Yuan Jochen Kang.

(Imported from upstream's 8f43c80bfac15544820739bf035df946eeb603e8)

Change-Id: If247d5bf1d792eb7c6dc179b606ed21ea0ccdbb8
Reviewed-on: https://boringssl-review.googlesource.com/7743
Reviewed-by: David Benjamin <davidben@google.com>
2016-04-26 17:12:01 +00:00
David Benjamin
b7c5e84847 Fix some malloc test failures.
These only affect the tests.

Change-Id: If22d047dc98023501c771787b485276ece92d4a2
Reviewed-on: https://boringssl-review.googlesource.com/7573
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-28 17:17:32 +00:00
Brian Smith
dc6c1b8381 Fix build when using Visual Studio 2015 Update 1.
Many of the compatibility issues are described at
https://msdn.microsoft.com/en-us/library/mt612856.aspx. The macros
that suppressed warnings on a per-function basis no longer work in
Update 1, so replace them with #pragmas. Update 1 warns when |size_t|
arguments to |printf| are casted, so stop doing that casting.
Unfortunately, this requires an ugly hack to continue working in
MSVC 2013 as MSVC 2013 doesn't support "%zu". Finally, Update 1 has new
warnings, some of which need to be suppressed.

---

Updated by davidben to give up on suppressing warnings in crypto/x509 and
crypto/x509v3 as those directories aren't changed much from upstream. In each
of these cases, upstream opted just blindly initialize the variable, so do the
same. Also switch C4265 to level 4, per Microsoft's recommendation and work
around a bug in limits.h that happens to get fixed by Google include order
style.

(limits.h is sensitive to whether corecrt.h, pulled in by stddef.h and some
other headers, is included before it. The reason it affected just one file is
we often put the file's header first, which means base.h is pulling in
stddef.h. Relying on this is ugly, but it's no worse than what everything else
is doing and this doesn't seem worth making something as tame as limits.h so
messy to use.)

Change-Id: I02d1f935356899f424d3525d03eca401bfa3e6cd
Reviewed-on: https://boringssl-review.googlesource.com/7480
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-25 21:39:52 +00:00
David Benjamin
eebfd896fe Don't shift serial number into sign bit
(Imported from upstream's 01c32b5e448f6d42a23ff16bdc6bb0605287fa6f.)

Change-Id: Ib52278dbbac1ed1ad5c80f0ad69e34584d411cec
Reviewed-on: https://boringssl-review.googlesource.com/7461
Reviewed-by: Steven Valdez <svaldez@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-17 18:23:49 +00:00
David Benjamin
a2f2bc3a40 Align with upstream's error strings, take two.
I messed up a few of these.

ASN1_R_UNSUPPORTED_ALGORITHM doesn't exist. X509_R_UNSUPPORTED_ALGORITHM does
exist as part of X509_PUBKEY_set, but the SPKI parser doesn't emit this. (I
don't mind the legacy code having really weird errors, but since EVP is now
limited to things we like, let's try to keep that clean.) To avoid churn in
Conscrypt, we'll keep defining X509_R_UNSUPPORTED_ALGORITHM, but not actually
do anything with it anymore.  Conscrypt was already aware of
EVP_R_UNSUPPORTED_ALGORITHM, so this should be fine. (I don't expect
EVP_R_UNSUPPORTED_ALGORITHM to go away. The SPKI parsers we like live in EVP
now.)

A few other ASN1_R_* values didn't quite match upstream, so make those match
again. Finally, I got some of the rsa_pss.c values wrong. Each of those
corresponds to an (overly specific) RSA_R_* value in upstream. However, those
were gone in BoringSSL since even the initial commit. We placed the RSA <-> EVP
glue in crypto/evp (so crypto/rsa wouldn't depend on crypto/evp) while upstream
placed them in crypto/rsa.

Since no one seemed to notice the loss of RSA_R_INVALID_SALT_LENGTH, let's undo
all the cross-module errors inserted in crypto/rsa. Instead, since that kind of
specificity is not useful, funnel it all into X509_R_INVALID_PSS_PARAMETERS
(formerly EVP_R_INVALID_PSS_PARAMETERS, formerly RSA_R_INVALID_PSS_PARAMETERS).

Reset the error codes for all affected modules.

(That our error code story means error codes are not stable across this kind of
refactoring is kind of a problem. Hopefully this will be the last of it.)

Change-Id: Ibfb3a0ac340bfc777bc7de6980ef3ddf0a8c84bc
Reviewed-on: https://boringssl-review.googlesource.com/7458
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-15 16:02:12 +00:00
David Benjamin
fb8e678897 Match upstream's error codes for the old sigalg code.
People seem to condition on these a lot. Since this code has now been moved
twice, just make them all cross-module errors rather than leave a trail of
renamed error codes in our wake.

Change-Id: Iea18ab3d320f03cf29a64a27acca119768c4115c
Reviewed-on: https://boringssl-review.googlesource.com/7431
Reviewed-by: Emily Stark (Dunn) <estark@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-11 21:15:47 +00:00
Adam Langley
d323f4b1e1 Bring back |verify_store|.
This was dropped in d27441a9cb due to lack
of use, but node.js now needs it.

Change-Id: I1e207d4b46fc746cfae309a0ea7bbbc04ea785e8
Reviewed-on: https://boringssl-review.googlesource.com/7270
Reviewed-by: David Benjamin <davidben@google.com>
2016-03-02 15:57:27 +00:00
David Benjamin
17727c6843 Move all signature algorithm code to crypto/x509.
All the signature algorithm logic depends on X509_ALGOR. This also
removes the X509_ALGOR-based EVP functions which are no longer used
externally. I think those APIs were a mistake on my part. The use in
Chromium was unnecessary (and has since been removed anyway). The new
X.509 stack will want to process the signatureAlgorithm itself to be
able to enforce policies on it.

This also moves the RSA_PSS_PARAMS bits to crypto/x509 from crypto/rsa.
That struct is also tied to crypto/x509. Any new RSA-PSS code would
have to use something else anyway.

BUG=499653

Change-Id: I6c4b4573b2800a2e0f863d35df94d048864b7c41
Reviewed-on: https://boringssl-review.googlesource.com/7025
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:39:02 +00:00
David Benjamin
93a69b4f8f Move X.509 signature algorithm tests to the crypto/x509 layer.
This is in preparation for moving the logic itself to crypto/x509, so
the lower-level functions will not be as readily available.

Change-Id: I6507b895317df831ab11d0588c5b09bbb2aa2c24
Reviewed-on: https://boringssl-review.googlesource.com/7023
Reviewed-by: Adam Langley <agl@google.com>
2016-02-26 22:38:50 +00:00
Steven Valdez
3f81b607fe Fix missing ok=0 with cert verification.
Also avoid using "i" in X509_cert_verify as a loop counter, trust
outcome and as an error ordinal.

(Imported from upstream's a3baa171053547488475709c7197592c66e427cf)

Change-Id: I4b0b542ffacf7fa861c93c8124b334c0aacc3c17
Reviewed-on: https://boringssl-review.googlesource.com/7222
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-25 20:43:58 +00:00
David Benjamin
06c5fb4512 Revert "Fix missing ok=0 with cert verification."
This reverts commit b0576889fa.

This broke x509_test.

Change-Id: Idbb60df9ca0a8ce727931f8e35e99bbd0f08c3c1
Reviewed-on: https://boringssl-review.googlesource.com/7221
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-25 20:23:38 +00:00
Steven Valdez
b0576889fa Fix missing ok=0 with cert verification.
Also avoid using "i" in X509_cert_verify as a loop counter, trust
outcome and as an error ordinal.

(Imported from upstream's a3baa171053547488475709c7197592c66e427cf)

Change-Id: I492afdbaa5017bcf00a0412033cf99fca3fe9401
Reviewed-on: https://boringssl-review.googlesource.com/7218
Reviewed-by: David Benjamin <davidben@google.com>
2016-02-25 20:01:07 +00:00