Commit graph

8971 commits

Author SHA1 Message Date
Hanno Becker 3d699e43ea SSL/TLS client: Remove old session ticket on renegotiation
Context: During a handshake, the SSL/TLS handshake logic constructs
an instance of ::mbedtls_ssl_session representing the SSL session
being established. This structure contains information such as the
session's master secret, the peer certificate, or the session ticket
issues by the server (if applicable).

During a renegotiation, the new session is constructed aside the existing
one and destroys and replaces the latter only when the renegotiation is
complete. While conceptually clear, this means that during the renegotiation,
large pieces of information such as the peer's CRT or the session ticket
exist twice in memory, even though the original versions are removed
eventually.

This commit starts removing this memory inefficiency by freeing the old
session's SessionTicket before the one for the new session is allocated.
2019-06-05 14:25:28 +01:00
Hanno Becker 80e92ad43a Adapt ChangeLog 2019-06-05 13:18:05 +01:00
Hanno Becker 2fa5e73d8d Modify existing X.509 test for also test new copyless API
The existing test `x509parse_crt()` for X.509 CRT parsing
so far used the generic parsing API `mbedtls_x509_crt_parse()`
capable of parsing both PEM encoded and DER encoded certficates,
but was actually only used with DER encoded input data. Moreover,
as the purpose of the test is the testing of the core DER X.509 parsing
functionality, not the PEM vs. DER dispatch (which is now already tested
in the various `x509_crt_info()` tests), the call can be replaced with a
direct call to `mbedtls_x509_parse_crt_der()`.

This commit does that, and further adds to the test an analogous
call to the new API `mbedtls_x509_parse_crt_der_nocopy()` to test
copyless parsing of X.509 certificates.
2019-06-05 13:16:55 +01:00
Hanno Becker d58b133276 Add test for mbedtls_x509_parse_file() with DER encoded CRT 2019-06-05 13:16:55 +01:00
Hanno Becker aa8665ac26 Add a new X.509 API call for copy-less parsing of CRTs
Context:
The existing API `mbedtls_x509_parse_crt_der()` for parsing DER
encoded X.509 CRTs unconditionally makes creates a copy of the
input buffer in RAM. While this comes at the benefit of easy use,
-- specifically: allowing the user to free or re-use the input
buffer right after the call -- it creates a significant memory
overhead, as the CRT is duplicated in memory (at least temporarily).
This might not be tolerable a resource constrained device.

As a remedy, this commit adds a new X.509 API call

   `mbedtls_x509_parse_crt_der_nocopy()`

which has the same signature as `mbedtls_x509_parse_crt_der()`
and almost the same semantics, with one difference: The input
buffer must persist and be unmodified for the lifetime of the
established instance of `mbedtls_x509_crt`, that is, until
`mbedtls_x509_crt_free()` is called.
2019-06-05 13:16:55 +01:00
Simon Butcher 21d1c32b2b Merge remote-tracking branch 'origin/pr/574' into baremetal 2019-06-04 15:08:32 +01:00
Simon Butcher d2cd0b8041 Merge remote-tracking branch 'origin/pr/567' into baremetal 2019-06-04 15:06:20 +01:00
Simon Butcher 2b2cbb5c0f Merge remote-tracking branch 'origin/pr/570' into baremetal 2019-06-04 15:06:07 +01:00
Simon Butcher 786ff7911c Merge remote-tracking branch 'origin/pr/568' into baremetal 2019-06-04 15:05:42 +01:00
Hanno Becker 08c78a2e32 UDP proxy: Don't attempt to dissect dgram into records when dropping
To prevent dropping the same message over and over again, the UDP proxy
test application programs/test/udp_proxy _logically_ maintains a mapping
from records to the number of times the record has already been dropped,
and stops dropping once a configurable threshold (currently 2) is passed.

However, the actual implementation deviates from this logical view
in two crucial respects:
- To keep the implementation simple and independent of
  implementations of suitable map interfaces, it only counts how
  many times a record of a given _size_ has been dropped, and
  stops dropping further records of that size once the configurable
  threshold is passed. Of course, this is not fail-proof, but a
  good enough approximation for the proxy, and it allows to use
  an inefficient but simple array for the required map.
- The implementation mixes datagram lengths and record lengths:
  When deciding whether it is allowed to drop a datagram, it
  uses the total datagram size as a lookup index into the map
  counting the number of times a package has been dropped. However,
  when updating this map, the UDP proxy traverses the datagram
  record by record, and updates the mapping at the level of record
  lengths.

Apart from this inconsistency, the introduction of the Connection ID
feature leads to yet another problem: The CID length is not part of
the record header but dynamically negotiated during (potentially
encrypted!) handshakes, and it is hence impossible for a passive traffic
analyzer (in this case our UDP proxy) to reliably parse record headers;
especially, it isn't possible to reliably infer the length of a record,
nor to dissect a datagram into records.

The previous implementation of the UDP proxy was not CID-aware and
assumed that the record length would always reside at offsets 11, 12
in the DTLS record header, which would allow it to iterate through
the datagram record by record. As mentioned, this is no longer possible
for CID-based records, and the current implementation can run into
a buffer overflow in this case (because it doesn't validate that
the record length is not larger than what remains in the datagram).

This commit removes the inconsistency in datagram vs. record length
and resolves the buffer overflow issue by not attempting any dissection
of datagrams into records, and instead only counting how often _datagrams_
of a particular size have been dropped.

There is only one practical situation where this makes a difference:
If datagram packing is used by default but disabled on retransmission
(which OpenSSL has been seen to do), it can happen that we drop a
datagram in its initial transmission, then also drop some of its records
when they retransmitted one-by-one afterwards, yet still keeping the
drop-counter at 1 instead of 2. However, even in this situation, we'll
correctly count the number of droppings from that point on and eventually
stop dropping, because the peer will not fall back to using packing
and hence use stable record lengths.
2019-06-04 13:04:28 +01:00
Hanno Becker 7bf7710f40 Remove reference to outdated compile-time option 2019-06-04 09:44:25 +01:00
Manuel Pégourié-Gonnard f3c43dde54 Merge branch 'mbedtls-2.16' into baremetal
* mbedtls-2.16:
  test: Always use `make clean` by itself
  list-symbols.sh: if the build fails, print the build transcript
  Document "check-names.sh -v"
  all.sh: invoke check-names.sh in print-trace-on-exit mode
  Print a command trace if the check-names.sh exits unexpectedly
  Only use submodule if present
  Update change log
  Reword ssl_conf_max_frag_len documentation for clarity
  Ignore more generated files: seedfile, apidoc
  Improve .gitignore grouping and documentation
  Generate tags for Vi, for Emacs and with Global
2019-06-04 09:39:51 +02:00
Hanno Becker 5dbcc9f441 Introduce specific error for ver/cfg mismatch on deserialization
This commit introduces a new SSL error code

  `MBEDTLS_ERR_SSL_VERSION_MISMATCH`

which can be used to indicate operation failure due to a
mismatch of version or configuration.

It is put to use in the implementation of `mbedtls_ssl_session_load()`
to signal the attempt to de-serialize a session which has been serialized
in a build of Mbed TLS using a different version or configuration.
2019-06-03 13:01:21 +01:00
Hanno Becker f78af3779a Improve test for detection of ver/cfg corruption in serialized data
This commit improves the test exercising the behaviour of
session deserialization when facing an unexpected version
or config, by testing ver/cfg corruption at any bit in the
ver/cfg header of the serialized data; previously, it had
only tested the first bit of each byte.
2019-06-03 12:49:36 +01:00
Hanno Becker 08ec129dd8 Use US spelling 'serialize' instead of UK spelling 'serialise' 2019-06-03 12:49:36 +01:00
Hanno Becker baf968cf69 Use def'n consts for bits in config-identifier of serialized data 2019-06-03 12:49:09 +01:00
Hanno Becker b36db4f368 Note that ver+fmt bytes in serialized data must not be removed 2019-06-03 12:49:09 +01:00
Hanno Becker 26829e99b2 Improve doc'n of config-identifying bitfield in serialized session 2019-06-03 12:48:50 +01:00
Hanno Becker 1d8b6d7b12 Session serialization: Fail with BAD_INPUT_DATA if buffer too small 2019-06-03 12:48:31 +01:00
Hanno Becker cb9ba0f43c Use consistent spelling of 'serialise/serialize' in SSL test suite 2019-06-03 12:48:16 +01:00
Hanno Becker f99ec2618d Add negative tests for unexpected ver/cfg in session deserialization 2019-06-03 12:48:16 +01:00
Hanno Becker 41527624f6 Encode relevant parts of the config in serialized session header
This commit makes use of the added space in the session header to
encode the state of those parts of the compile-time configuration
which influence the structure of the serialized session in the
present version of Mbed TLS. Specifically, these are
- the options which influence the presence/omission of fields
  from mbedtls_ssl_session (which is currently shallow-copied
  into the serialized session)
- the setting of MBEDTLS_X509_CRT_PARSE_C, which determines whether
  the serialized session contains a CRT-length + CRT-value pair after
  the shallow-copied mbedtls_ssl_session instance.
- the setting of MBEDTLS_SSL_SESSION_TICKETS, which determines whether
  the serialized session contains a session ticket.
2019-06-03 12:48:16 +01:00
Hanno Becker 557fe9ffde Add configuration identifier to serialized SSL sessions
This commit adds space for two bytes in the header of serizlied
SSL sessions which can be used to determine the structure of the
remaining serialized session in the respective version of Mbed TLS.

Specifically, if parts of the session depend on whether specific
compile-time options are set or not, the setting of these options
can be encoded in the added space.

This commit doesn't yet make use of the fields.
2019-06-03 12:46:39 +01:00
Hanno Becker b5352f0489 Add Mbed TLS version to SSL sessions
The format of serialized SSL sessions depends on the version and the
configuration of Mbed TLS; attempts to restore sessions established
in different versions and/or configurations lead to undefined behaviour.

This commit adds an 3-byte version header to the serialized session
generated and cleanly fails ticket parsing in case a session from a
non-matching version of Mbed TLS is presented.
2019-06-03 12:46:23 +01:00
Jaeden Amero 08f363baa9 Merge remote-tracking branch 'origin/pr/2666' into mbedtls-2.16
* origin/pr/2666:
  test: Always use `make clean` by itself
2019-06-03 09:56:44 +01:00
Manuel Pégourié-Gonnard c4f5080b34 Re-enable test that now works with new format
Previously the test didn't work because of embedded pointer values that
are not predictable. Now it works as we no longer serialize such values.
2019-06-03 10:53:47 +02:00
Manuel Pégourié-Gonnard f8c355a012 Adapt buffering test to new ticket size
The size of the ticket used in this test dropped from 192 to 143 bytes, so
move all sizes used in this test down 50 bytes. Also, we now need to adapt the
server response size as the default size would otherwise collide with the new
mtu value.
2019-06-03 10:15:07 +02:00
Manuel Pégourié-Gonnard 60a4299bbf Add new ABI-independent format for serialization 2019-06-03 10:15:07 +02:00
Manuel Pégourié-Gonnard 35ccdbb636 Normalize spelling to serialiZation
We have explicit recommendations to use US spelling for technical writing, so
let's apply this to code as well for uniformity. (My fingers tend to prefer UK
spelling, so this needs to be fixed in many places.)

sed -i 's/\([Ss]eriali\)s/\1z/g' **/*.[ch] **/*.function **/*.data ChangeLog
2019-06-03 09:55:16 +02:00
Manuel Pégourié-Gonnard e0cd1d0184 Improve documentation 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 4bb1b99c7f Demonstrate safe usage (zeroize) in ssl_client2 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 2d8847e84d Add a ChangeLog entry for session serialisation 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 749312fb8a Fix undeclared dependency on FS_IO in test code
Found by 'all.sh test_no_platform' and by 'tests/scripts/test-ref-configs.pl'.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard d1a5451fb5 Fix style issues and typos in test code 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 081b15231f Fix another wrong check for errors in test code 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 5363e1f496 Add list of coupled functions to struct definition 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 2a62a05688 Add test that save-load is the identity
This test works regardless of the serialisation format and embedded pointers
in it, contrary to the load-save test, though it requires more maintenance of
the test code (sync the member list with the struct definition).
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard aab6204dc1 Fix populate_session() and its usage in tests
Not checking the return value allowed a bug to go undetected, fix the bug and
check the return value.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 5709811dd2 Add test for session_load() from small buffers
This uncovered a bug that led to a double-free (in practice, in general could
be free() on any invalid value): initially the session structure is loaded
with `memcpy()` which copies the previous values of pointers peer_cert and
ticket to heap-allocated buffers (or any other value if the input is
attacker-controlled). Now if we exit before we got a chance to replace those
invalid values with valid ones (for example because the input buffer is too
small, or because the second malloc() failed), then the next call to
session_free() is going to call free() on invalid pointers.

This bug is fixed in this commit by always setting the pointers to NULL right
after they've been read from the serialised state, so that the invalid values
can never be used.

(An alternative would be to NULL-ify them when writing, which was rejected
mostly because we need to do it when reading anyway (as the consequences of
free(invalid) are too severe to take any risk), so doing it when writing as
well is redundant and a waste of code size.)

Also, while thinking about what happens in case of errors, it became apparent
to me that it was bad practice to leave the session structure in an
half-initialised state and rely on the caller to call session_free(), so this
commit also ensures we always clear the structure when loading failed.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 98fccc3f6a Add test for session_save() on small buffers 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 1ba5c68503 Disable test for load-save identity
This test appeared to be passing for the wrong reason, it's not actually not
appropriate for the current implementation. The serialised data contains
values of pointers to heap-allocated buffers. There is no reason these should
be identical after a load-save pair. They just happened to be identical when I
first ran the test due to the place of session_free() in the test code and the
fact that the libc's malloc() reused the same buffers. The test no longer
passes if other malloc() implementations are used (for example, when compiling
with asan which avoids re-using the buffer, probably for better error
detection).

So, disable this test for now (we can re-enable it when we changed how
sessions are serialised, which will be done in a future PR, hence the name of
the dummy macro in depends_on). In the next commit we're going to add a test
that save-load is the identity instead - which will be more work in testing as
it will require checking each field manually, but at least is reliable.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 16f6bb1aa3 Improve load-save test with tickets and certs 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard dfa5a7ae76 Start adding unit test for session serialisation
This initial test ensures that a load-save function is the identity. It is so
far incomplete in that it only tests sessions without tickets or certificate.
This will be improved in the next commits.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 32ce596c35 Improve save API by always updating olen
This allows callers to discover what an appropriate size is. Otherwise they'd
have to either try repeatedly, or allocate an overly large buffer (or some
combination of those).

Adapt documentation an example usage in ssl_client2.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 57a348ba8c Add tests for session copy without serialisation 2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 37a5324c74 Add mbedtls_ssl_get_session_pointer()
Avoid useless copy with mbedtls_ssl_get_session() before serialising.

Used in ssl_client2 for testing and demonstrating usage, but unfortunately
that means mbedtls_ssl_get_session() is no longer tested, which will be fixed
in the next commit.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard fbb44a422f Save session in serialised form in ssl_client2.
This provides basic testing for the session (de)serialisation functions, as
well as an example of how to use them.

Tested locally with tests/ssl-opt.sh -f '^Session resume'.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard ef4ae611e4 Add support for serialisation session with ticket
On client side, this is required for the main use case where of serialising a
session for later resumption, in case tickets are used.

On server side, this doesn't change much as ticket_len will always be 0.

This unblocks testing the functions by using them in ssl_client2, which will
be done in the next commit.
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 91f4ca2ed1 Move session save/load function to ssl_tls.c
This finishes making these functions public. Next step is to get them tested,
but there's currently a blocker for that, see next commit (and the commit
after it for tests).
2019-06-03 09:51:08 +02:00
Manuel Pégourié-Gonnard 2843fe10b9 Declare and document session save/load functions
The next commit with make the implementation publicly available as well.

For now the API is kept unchanged. The save function API has a serious drawback in that the user
must guess what an appropriate buffer size is.
Internally so far this didn't matter because we were only using that API for
ticket creation, and tickets are written to the SSL output buffer whose size
is fixed anyway, but for external users this might not be suitable. Improving
that is left for later.

Also, so far the functions are defined unconditionally. Whether we want to
re-use existing flags or introduce a new one is left for later.

Finally, currently suggested usage of calling get_session() then
session_save() is memory-inefficient in that get_session() already makes a
copy. I don't want to recommend accessing `ssl->session` directly as we want
to prohibit direct access to struct member in the future. Providing a clean
and efficient way is also left to a later commit.
2019-06-03 09:51:08 +02:00