This commit handles occurrences of case 2 and 3 in the following list:
1. Some DTLS-specific code with no TLS-specific code (most frequent)
2. Some specific code for each protocol
3. Some TLS-specific code with no DTLS-specific code (least frequent)
Case 3 previously had a weird structure in that the TLS-specific code was
always present, but the if structure was conditional on DTLS being enabled.
This is changed by this commit to a more logical structure where both the code
and the test are conditional on TLS being enabled.
Case 2 doesn't require any change in the code structure in general. However,
there is one occurrence where the if/else structure is simplified to assigning
the result of a boolean operation, and one occurrence where I also noticed a
useless use of `ssl_ep_len()` in a TLS-specific branch, that I turned to the
constant 0 as it makes more sense.
Case 1 will be handled in the next commit, as it can easily be handled in an
automated way - only cases 2 and 3 (sometimes) required manual intervention.
The list of occurrences for cases 2 and 3 was established manually by looking
for occurrences of '= MBEDTLS_SSL_TRANSPORT_' in the code and manually
checking if there was a TLS-specific branch.
New sizes (see previous commit for the measuring script):
```
both
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17156 0 0 17156 4304 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17649 0 0 17649 44f1 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
39286 60 0 39346 99b2 ssl_tls.o (ex library/libmbedtls.a)
88874 60 600 89534 15dbe (TOTALS)
DTLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17068 0 0 17068 42ac ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17553 0 0 17553 4491 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
38499 60 0 38559 969f ssl_tls.o (ex library/libmbedtls.a)
87903 60 600 88563 159f3 (TOTALS)
TLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
14912 0 0 14912 3a40 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
15868 0 0 15868 3dfc ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
27619 60 0 27679 6c1f ssl_tls.o (ex library/libmbedtls.a)
73182 60 600 73842 12072 (TOTALS)
```
And use those tools in a few places. For now the purpose is just to validate
those tools before using them in all occurrences of transport-specific code.
The effect of these changes was measured with the following script:
```
set -eu
build() {
printf "\n$1\n"
CC=arm-none-eabi-gcc CFLAGS='-Werror -Os -march=armv6-m -mthumb' \
AR=arm-none-eabi-ar LD=arm-none-eabi-ld make clean lib >/dev/null
arm-none-eabi-size -t library/libmbedtls.a
}
git checkout -- include/mbedtls/config.h
scripts/config.pl unset MBEDTLS_NET_C
scripts/config.pl unset MBEDTLS_TIMING_C
scripts/config.pl unset MBEDTLS_FS_IO
scripts/config.pl unset MBEDTLS_ENTROPY_NV_SEED
scripts/config.pl set MBEDTLS_NO_PLATFORM_ENTROPY
build "both"
scripts/config.pl unset MBEDTLS_SSL_PROTO_TLS
build "DTLS-only"
scripts/config.pl set MBEDTLS_SSL_PROTO_TLS
scripts/config.pl unset MBEDTLS_SSL_PROTO_DTLS
scripts/config.pl unset MBEDTLS_SSL_DTLS_HELLO_VERIFY
scripts/config.pl unset MBEDTLS_SSL_DTLS_ANTI_REPLAY
scripts/config.pl unset MBEDTLS_SSL_DTLS_BADMAC_LIMIT
scripts/config.pl unset MBEDTLS_SSL_DTLS_CLIENT_PORT_REUSE
build "TLS-only"
git checkout -- include/mbedtls/config.h
```
The output of the script is as follows:
```
both
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17160 0 0 17160 4308 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17637 0 0 17637 44e5 ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
39322 60 0 39382 99d6 ssl_tls.o (ex library/libmbedtls.a)
88902 60 600 89562 15dda (TOTALS)
DTLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
17072 0 0 17072 42b0 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
17565 0 0 17565 449d ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
38953 60 0 39013 9865 ssl_tls.o (ex library/libmbedtls.a)
88373 60 600 89033 15bc9 (TOTALS)
TLS-only
text data bss dec hex filename
1820 0 4 1824 720 debug.o (ex library/libmbedtls.a)
0 0 0 0 0 net_sockets.o (ex library/libmbedtls.a)
548 0 0 548 224 ssl_cache.o (ex library/libmbedtls.a)
11155 0 596 11751 2de7 ssl_ciphersuites.o (ex library/libmbedtls.a)
14916 0 0 14916 3a44 ssl_cli.o (ex library/libmbedtls.a)
460 0 0 460 1cc ssl_cookie.o (ex library/libmbedtls.a)
15852 0 0 15852 3dec ssl_srv.o (ex library/libmbedtls.a)
800 0 0 800 320 ssl_ticket.o (ex library/libmbedtls.a)
27623 60 0 27683 6c23 ssl_tls.o (ex library/libmbedtls.a)
73174 60 600 73834 1206a (TOTALS)
```
It can be seen that a DTLS-only build is now starting to be a bit smaller than
a dual-mode build, which is the purpose of the new build option.
For now the option has no effect.
Adapted existing example config files. The fact that I needed to do this
highlights that this is a slightly incompatible change: existing users need to
update their existing custom configs (if standalone as opposed to based on the
default config) in order to still get the same behaviour.
The alternative would be to have a negative config option (eg NO_TLS or
DTLS_ONLY) but this doesn't fit as nicely with the existing options, so
hopefully the minor incompatibility is acceptable.
I don't think it's worth adding a new component to all.sh:
- builds with both DTLS and TLS are done in the default (and full) config
- TLS-only builds are done with eg config-suite-b.h in test-ref-configs
- a DTLS-only build is done with config-thread.h in test-ref-configs
- builds with none of them (and SSL_TLS_C enabled) are forbidden
* 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
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.
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.
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.
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.
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.
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.
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
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).
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.
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.
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.
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.
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.
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'.
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.
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).
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.
When running make with parallelization, running both "clean" and "lib"
with a single make invocation can lead to each target building in
parallel. It's bad if lib is partially done building something, and then
clean deletes what was built. This can lead to errors later on in the
lib target.
$ make -j9 clean lib
CC aes.c
CC aesni.c
CC arc4.c
CC aria.c
CC asn1parse.c
CC ./library/error.c
CC ./library/version.c
CC ./library/version_features.c
AR libmbedcrypto.a
ar: aes.o: No such file or directory
Makefile:120: recipe for target 'libmbedcrypto.a' failed
make[2]: *** [libmbedcrypto.a] Error 1
Makefile:152: recipe for target 'libmbedcrypto.a' failed
make[1]: *** [libmbedcrypto.a] Error 2
Makefile:19: recipe for target 'lib' failed
make: *** [lib] Error 2
make: *** Waiting for unfinished jobs....
To avoid this sort of trouble, always invoke clean by itself without
other targets throughout the library. Don't run clean in parallel with
other rules. The only place where clean was run in parallel with other
targets was in list-symbols.sh.
