In many places in TLS handling, some code detects a fatal error, sends
a fatal alert message, and returns to the caller. If sending the alert
fails, then return the error that triggered the alert, rather than
overriding the return status. This effectively causes alert sending
failures to be ignored. Formerly the code was inconsistently sometimes
doing one, sometimes the other.
In general ignoring the alert is the right thing: what matters to the
caller is the original error. A typical alert failure is that the
connection is already closed.
One case which remains not handled correctly is if the alert remains
in the output buffer (WANT_WRITE). Then it won't be sent, or will be
truncated. We'd need to either delay the application error or record
the write buffering notice; to be done later.
The TLS client and server code was usually closing the connection in
case of a fatal error without sending an alert. This commit adds
alerts in many cases.
Added one test case to detect that we send the alert, where a server
complains that the client's certificate is from an unknown CA (case
tracked internally as IOTSSL-1330).
Fix an incorrect condition in ssl_check_ctr_renegotiate() that compared
64 bits of record counter instead of 48 bits as described in RFC 6347
Section 4.3.1. This would cause the function's return value to be
occasionally incorrect and the renegotiation routines to be triggered
at unexpected times.
Fixes many typos, and errors in comments.
* Clarifies many comments
* Grammar correction in config.pl help text
* Removed comment about MBEDTLS_X509_EXT_NS_CERT_TYPE.
* Comment typo fix (Dont => Don't)
* Comment typo fix (assure => ensure)
* Comment typo fix (byes => bytes)
* Added citation for quoted standard
* Comment typo fix (one complement => 1's complement)
The is some debate about whether to prefer "one's complement", "ones'
complement", or "1's complement". The more recent RFCs related to TLS
(RFC 6347, RFC 4347, etc) use " 1's complement", so I followed that
convention.
* Added missing ")" in comment
* Comment alignment
* Incorrect comment after #endif
Certificates with unsupported algorithms in the certificate chain
prevented verification even if a certificate before the unsupported
ones was already trusted.
We change the behaviour to ignoring every certificate with unknown
(unsupported) signature algorithm oid when parsing the certificate
chain received from the peer.
Separates platform time abstraction into it's own header from the
general platform abstraction as both depend on different build options.
(MBEDTLS_PLATFORM_C vs MBEDTLS_HAVE_TIME)
Since the buffer is used in a few places, it seems Clang isn't clever
enough to realise that the first byte is never touched. So, even though
the function has a correct null check for ssl->handshake, Clang
complains. Pulling the handshake type out into its own variable is
enough for Clang's analysis to kick in though.
The callback typedefs defined for mbedtls_ssl_set_bio() and
mbedtls_ssl_set_timer_cb() were not used consistently where the callbacks were
referenced in structures or in code.
After the record contents are decompressed, in_len is no longer
accessed directly, only in_msglen is accessed. in_len is only read by
ssl_parse_record_header() which happens before ssl_prepare_record_contents().
This is also made clear by the fact that in_len is not touched after
decrypting anyway, so if it was accessed after that it would be wrong unless
decryption is used - as this is not the case, it show in_len is not accessed.
When the peer retransmits a flight with many record in the same datagram, and
we already saw one of the records in that datagram, we used to drop the whole
datagram, resulting in interoperability failure (spurious handshake timeouts,
due to ignoring record retransmitted by the peer) with some implementations
(issues with Chrome were reported).
So in those cases, we want to only drop the current record, and look at the
following records (if any) in the same datagram. OTOH, this is not something
we always want to do, as sometime the header of the current record is not
reliable enough.
This commit introduces a new return code for ssl_parse_header() that allows to
distinguish if we should drop only the current record or the whole datagram,
and uses it in mbedtls_ssl_read_record()
fixes#345
* iotssl-519-asn1write-overflows-restricted:
Fix other int casts in bounds checking
Fix other occurrences of same bounds check issue
Fix potential buffer overflow in asn1write
fixes#310
Actually all key exchanges that use a certificate use signatures too, and
there is no key exchange that uses signatures but no cert, so merge those two
flags.
Not a security issue as here we know the buffer is large enough (unless
something else if badly wrong in the code), and the value cast to int is less
than 2^16 (again, unless issues elsewhere).
Still changing to a more correct check as a matter of principle
* development: (73 commits)
Bump yotta dependencies version
Fix typo in documentation
Corrected misleading fn description in ssl_cache.h
Corrected URL/reference to MPI library
Fix yotta dependencies
Fix minor spelling mistake in programs/pkey/gen_key.c
Bump version to 2.1.2
Fix CVE number in ChangeLog
Add 'inline' workaround where needed
Fix references to non-standard SIZE_T_MAX
Fix yotta version dependencies again
Upgrade yotta dependency versions
Fix compile error in net.c with musl libc
Add missing warning in doc
Remove inline workaround when not useful
Fix macroization of inline in C++
Changed attribution for Guido Vranken
Merge of IOTSSL-476 - Random malloc in pem_read()
Fix for IOTSSL-473 Double free error
Fix potential overflow in CertificateRequest
...
Conflicts:
include/mbedtls/ssl_internal.h
library/ssl_cli.c
- "master secret" is the usual name
- move key block arg closer to the related lengths
- document lengths
Also fix some trailing whitespace while at it
This is more consistent, as it doesn't make any sense for a user to be able to
set up an EC J-PAKE password with TLS if the corresponding key exchange is
disabled.
Arguably this is what we should de for other key exchanges as well instead of
depending on ECDH_C etc, but this is an independent issue, so let's just do
the right thing with the new key exchange and fix the other ones later. (This
is a marginal issue anyway, since people who disable all ECDH key exchange are
likely to also disable ECDH_C in order to minimize footprint.)
Especially for resumed handshake, it's entirely possible for an epoch=0
ClientHello to be retransmitted or arrive so late that the server is already
at epoch=1. There is no good way to detect whether it's that or a reconnect.
However:
- a late ClientHello seems more likely that client going down and then up
again in the middle of a handshake
- even if that's the case, we'll time out on that handshake soon enough
- we don't want to break handshake flows that used to work
So the safest option is to not treat that as a reconnect.
Don't depend on srv.c in config.h, but add explicit checks. This is more
in line with other options that only make sense server-side, and also it
allows to test full config minus srv.c more easily.
Use a custom function that minimally parses the message an creates a reply
without the overhead of a full SSL context.
Also fix dependencies: needs DTLS_HELLO_VERIFY for the cookie types, and let's
also depend on SRV_C as is doesn't make sense on client.
This is not very useful for TLS as mbedtls_ssl_write() will automatically
fragment and return the length used, and the application should check for that
anyway, but this is useful for DTLS where mbedtls_ssl_write() returns an
error, and the application needs to be able to query the maximum length
instead of just guessing.
This is not required nor recommended by the protocol, and it's a layering
violation, but it's a know flaw in the protocol that you can't detect a PSK
auth error in any other way, so it is probably the right thing to do.
closes#227
We document that either of recv or recv_timeout may be NULL, but for TLS we
always used recv... Thanks Coverity for catching that.
(Not remotely trigerrable: local configuration.)
Also made me notice net_recv_timeout didn't do its job properly.
While at it, fix the following:
- on server with RSA_PSK, we don't want to set flags (client auth happens via
the PSK, no cert is expected).
- use safer tests (eg == OPTIONAL vs != REQUIRED)
- DTLS_HELLO_VERIFY no longer depends on SRV_C
- SSL_COOKIE_C no longer depends on DTLS_HELLO_VERIFY
Not that much work for us, and easier on users (esp. since it allows just
disabling SRV_C alone).
- Only the server needs to generate/parse tickets
- Only the client needs to store them
Also adjust prototype of ssl_conf_session_tickets() while at it.
Initially thought it would be per-connection, but since max_version is in conf
too, and you need to lower that for a fallback connection, the fallback flag
should be in the same place
A simple series of sed invocations.
This is the first step, purely internal changes. The conf substructure is not
ready to be shared between contexts yet.
