OpenSSL's BN_mul function had a single-word buffer underflow (see
576129cd72). We already independently
fixed this but, if we hadn't, ASan wouldn't have noticed because of
OPENSSL_malloc.
ASan has runtime hooks we can call to make it more accurate.
Change-Id: Ifc9c3837ece2bc456c5bdc960be707d7b1759904
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/35165
Reviewed-by: Adam Langley <agl@google.com>
Providing a size hint to the allocator is substantially faster,
especially as we already know/need the size for OPENSSL_cleanse.
We provide a weak symbol that falls back to free when a malloc with
sdallocx is not statically linked with BoringSSL.
Alternatives considered:
* Use dlsym(): This is prone to fail on statically linked binaries
without symbols. Additionally, the extra indirection adds call
overhead above and beyond the linker resolved technique we're using.
* Use CMake rules to identify whether sdallocx is available: Once the
library is built, we may link against a variety of malloc
implementations (not all of which may have sdallocx), so we need to
have a fallback when the symbol is unavailable.
Change-Id: I3a78e88fac5b6e5d4712aa0347d2ba6b43046e07
Reviewed-on: https://boringssl-review.googlesource.com/31784
Reviewed-by: Chris Kennelly <ckennelly@google.com>
Reviewed-by: Adam Langley <agl@google.com>
crypto/mem.c #include's <strings.h>, but doesn't use call any functions
from it.
Change-Id: If60b31be7dd6b347bcb077a59825a557a2492081
Reviewed-on: https://boringssl-review.googlesource.com/25964
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>
crypto/bio/bio_test.cc - I'm not sure where this was added for, but none
of the functions used there appear to have feature macros documented.
crypto/bio/printf.c - -std=c99 provides (v)snprintf.
crypto/lhash/lhash_test.cc - we no longer call rand_r.
crypto/mem.c - we no longer call strdup and -std=c99 provides (v)snprintf.
Apple messed up their headers and, if _POSIX_C_SOURCE is defined but
_DARWIN_C_SOURCE isn't, pthread.h no longer defines mach_port_t. They
then shipped a version of libc++ headers that is missing this fix, so
the build breaks:
bcc92d75df
If one uses XCode, they've hacked their pthread.h to provide mach_port_t
if defined(__cplusplus), but the standalone tools appear to be old and
missing this.
We can work around this by also defining _DARWIN_C_SOURCE in C++ files
that need _POSIX_C_SOURCE, but it appears none of these files actually
need it.
Change-Id: I5df9453730696100eb22b809febeb65053701322
Reviewed-on: https://boringssl-review.googlesource.com/20964
Reviewed-by: Adam Langley <agl@google.com>
Allocations by |OPENSSL_malloc| are prefixed with their length.
|OPENSSL_free| zeros the allocation before calling free(), eliminating
the need for a separate call to |OPENSSL_cleanse| for sensitive data.
This change will be followed up by the cleanup in
https://boringssl-review.googlesource.com/c/boringssl/+/19824.
Change-Id: Ie272f07e9248d7d78af9aea81dacec0fdb7484c4
Reviewed-on: https://boringssl-review.googlesource.com/19544
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
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>
X.509 functions and the like should not vary their behaviour based on
the configured locale, but tolower(3), strcasecmp(3) and strncasecmp(3)
change behaviour based on that.
For example, with tr_TR.utf8, 'I' is not the upper-case version of 'i'.
Change-Id: I896a285767ae0c22e6ce06b9908331c625e90af2
Reviewed-on: https://boringssl-review.googlesource.com/18412
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>
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>
clang-cl now supports enough of `#pragma intrinsic` that
it can use SecureZeroMemory() without an explicit intrin.h include.
This reverts https://boringssl-review.googlesource.com/#/c/8320/
BUG=chromium:592745
Change-Id: Ib766133f1713137bddd07654376a3b4888d4b0fb
Reviewed-on: https://boringssl-review.googlesource.com/11780
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>
Last month's canary for loop did not die in the coal mine of decrepit
toolchains. Make a note of this in STYLE.md so we know to start breeding
more of them. We can indeed declare index variables like it's 1999.
I haven't bothered to convert all of our for loops because that will be
tedious, but we can do it as we touch the code. Or if someone feels
really really bored.
BUG=47
Change-Id: Ib76c0767c1b509e825eac66f8c2e3ee2134e2493
Reviewed-on: https://boringssl-review.googlesource.com/8740
Reviewed-by: Adam Langley <agl@google.com>
I did the same change in NaCl in
https://codereview.chromium.org/2070533002/. I thought NaCl is the only
place where this was needed, but at least it's due to SecureZeroMemory()
again. So it's two files now, but at least there's only one function we
know of that needs this, and it's only called in three files total in
all projects used by Chromium.
BUG=chromium:592745
Change-Id: I07ed197869e26ec70c1f4b75d91fd64abae5015e
Reviewed-on: https://boringssl-review.googlesource.com/8320
Reviewed-by: David Benjamin <davidben@google.com>
There's a __pragma expression which allows this. Android builds us Windows with
MinGW for some reason, so we actually do have to tolerate non-MSVC-compatible
Windows compilers. (Clang for Windows is much more sensible than MinGW and
intentionally mimicks MSVC.)
MinGW doesn't understand MSVC's pragmas and warns a lot. #pragma warning is
safe to suppress, so wrap those to shush them. This also lets us do away with a
few ifdefs.
Change-Id: I1f5a8bec4940d4b2d947c4c1cc9341bc15ec4972
Reviewed-on: https://boringssl-review.googlesource.com/8236
Reviewed-by: Adam Langley <agl@google.com>
Switch one for loop to the new spelling as a canary. All our compilers seem to
support it fine, except GCC needs to be told to build with -std=c99. (And, upon
doing so, it'll require _XOPEN_SOURCE=700 for pthread_rwlock_t.)
We'll let this sit for a bit until it's gotten into downstreams without issue
and then open the floodgates.
BUG=47
Change-Id: I1c69d4b2df8206e0b55f30aa59b5874d82fca893
Reviewed-on: https://boringssl-review.googlesource.com/8235
Reviewed-by: Adam Langley <agl@google.com>
Use of strdup, close, lseek, read, and write prevent linking
statically againt libcmt.lib.
Change-Id: I04f7876ec0f03f29f000bbcc6b2ccdec844452d2
Reviewed-on: https://boringssl-review.googlesource.com/8010
Reviewed-by: David Benjamin <davidben@google.com>
These are not in upstream and were probably introduced on accident by stray vim
keystrokes.
Change-Id: I35f51f81fc37e75702e7d8ffc6f040ce71321b54
Reviewed-on: https://boringssl-review.googlesource.com/5490
Reviewed-by: Adam Langley <agl@google.com>
MinGW on Linux needs lowercase include files. On Windows this doesn't
matter since the filesystems are case-insensitive, but building
BoringSSL on Linux with MinGW has case-sensitive filesystems.
Change-Id: Id9c120d819071b041341fbb978352812d6d073bc
Reviewed-on: https://boringssl-review.googlesource.com/4090
Reviewed-by: Adam Langley <agl@google.com>
This avoids a conflict with the Chromium build system, which
defines WIN32_LEAN_AND_MEAN with a different value.
BUG=crbug.com/453196
Change-Id: Ia15ec7c20325c1961af4f32e5208266e5f846f35
Reviewed-on: https://boringssl-review.googlesource.com/3150
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Define WIN32_LEAN_AND_MEAN before including Windows Platform SDK
headers to preempt naming conflicts and to make the build faster. Avoid
including those headers in BoringSSL headers. Document that Platform
SDK 8.1 or later is required on Windows.
Change-Id: I907ada21dc722527ea37e839c71c5157455a7003
Reviewed-on: https://boringssl-review.googlesource.com/3100
Reviewed-by: Adam Langley <agl@google.com>
NaCl defines _POSIX_C_SOURCE on the command line for some reason, thus
we have to be defensive about defining it.
Change-Id: Icbc8afcb1ac0e0ca23b788b11ea911c3f55a8b7f
Reviewed-on: https://boringssl-review.googlesource.com/1891
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
_BSD_SOURCE has been deprecated (see bug). The manpage for printf
suggests that any _POSIX_C_SOURCE >= 200112L is also sufficient to bring
in the needed declarations and the bug reporter confirms that it's
sufficient for him.
https://code.google.com/p/chromium/issues/detail?id=419859
Change-Id: Ifc053f11c5aa1df35aae8e952d2c73a7f4599ec2
Reviewed-on: https://boringssl-review.googlesource.com/1890
Reviewed-by: Adam Langley <agl@google.com>
Windows doesn't have ssize_t, sadly. There's SSIZE_T, but defining an
OPENSSL_SSIZE_T seems worse than just using an int.
Change-Id: I09bb5aa03f96da78b619e551f92ed52ce24d9f3f
Reviewed-on: https://boringssl-review.googlesource.com/1352
Reviewed-by: Adam Langley <agl@google.com>
PNaCl builds BoringSSL with OPENSSL_NO_ASM, but the new OPENSSL_cleanse
was using inline assembly anyway. It appears that even though the inline
asm was empty, it still breaks the PNaCl build:
disallowed: inline assembly: call void asm sideeffect "", "r,~{memory}"(i8* %.asptr319), !dbg !96986
With this change, we don't have any compiler scarecrows for
OPENSSL_cleanse any longer when using OPENSSL_NO_ASM :( Maybe, one day,
we'll get memset_s in our base platform.
Change-Id: Ia359f6bcc2000be18a6f15de10fc683452151741
Reviewed-on: https://boringssl-review.googlesource.com/1353
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
Compilers have a bad habit of removing "superfluous" memset calls that
are trying to zero memory. For example, when memset()ing a buffer and
then free()ing it, the compiler might decide that the memset is
unobservable and thus can be removed.
Previously we tried to stop this by a) implementing memset in assembly
on x86 and b) putting the function in its own file for other platforms.
This change removes those tricks in favour of using asm directives to
scare the compiler away. As best as our compiler folks can tell, this is
sufficient and will continue to be so.
Change-Id: I40e0a62c3043038bafd8c63a91814a75a3c59269
Reviewed-on: https://boringssl-review.googlesource.com/1339
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>
PNaCl needs OPENSSL_NO_ASM to work and a couple of cases were missing
because it hasn't previously been tested.
Additionally, it defined _BSD_SOURCE and others on the command line,
causing duplicate definition errors when defined in source code.
It's missing readdir_r.
It uses newlib, which appears to use u_short in socket.h without ever
defining it.
Change-Id: Ieccfc7365723d0521f6327eebe9f44a2afc57406
Reviewed-on: https://boringssl-review.googlesource.com/1140
Reviewed-by: Adam Langley <agl@google.com>
Windows has different names for these functions and also doesn't have
the strings.h header in which they appear.
This change adds tiny wrapper functions for Windows.
Fix a bug in handling of 128 byte long PSK identity in
psk_client_callback.
OpenSSL supports PSK identities of up to (and including) 128 bytes in
length. PSK identity is obtained via the psk_client_callback,
implementors of which are expected to provide a NULL-terminated
identity. However, the callback is invoked with only 128 bytes of
storage thus making it impossible to return a 128 byte long identity and
the required additional NULL byte.
This CL fixes the issue by passing in a 129 byte long buffer into the
psk_client_callback. As a safety precaution, this CL also zeroes out the
buffer before passing it into the callback, uses strnlen for obtaining
the length of the identity returned by the callback, and aborts the
handshake if the identity (without the NULL terminator) is longer than
128 bytes.
David Benjamin
Jeremy Apthorp
Chris Kennelly
Aaron Green
Martin Kreichgauer
Adam Langley
Nico Weber
nmittler
David Benjamin
Brian Smith
Adam Langley