add mbedtls crypto support
ClosedPublic

Authored by jvijtiuk on Aug 9 2017, 11:24 AM.

Details

Summary

This patch adds support for mbedTLS as a crypto backend for libssh.
mbedTLS is an SSL/TLS library that has been designed to mainly be used
in embedded systems. It is loosely coupled and has a low memory
footprint. mbedTLS also provides a cryptography library (libmbedcrypto)
that can be used without the TLS modules.
The patch is unfortunately quite big, since several new files had to
be added.
DSA is disabled at compile time, since mbedTLS doesn't support DSA
Patch review and feedback would be appreciated, and if any issues or
suggestions appear, I'm willing to work on them.

Signed-off-by: Juraj Vijtiuk <juraj.vijtiuk@sartura.hr>

Test Plan
  • The patch has been tested with a Debug and MinSizeRel build, with

libssh unit tests, client tests and the pkd tests.

  • All the tests have been run with valgrind's memcheck, drd and helgrind

tools.

  • The examples/samplessh client works when built with the patch.

Diff Detail

Repository
rLIBSSH libssh
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
jvijtiuk created this revision.Aug 9 2017, 11:24 AM
jvijtiuk added a reviewer: asn.Aug 9 2017, 1:11 PM
asn requested changes to this revision.Aug 9 2017, 1:27 PM

libssh.h and server.h are public files. You cannot use

#ifdef HAVE_DSA

there. HAVE_DSA is defined in config.h which a libssh private header. You need to remove the ifdef!

Also I would like to avoid the #ifdef hell as much as possible in pki.c. It is fine for the function at the beginning which should retrun UNKOWN_TYPE for DSA but functions like pki_import_pubkey_buffer() should not have and #ifdef HAVE_DSA, instead pki_pubkey_build_dss() which is implemented by the pki_<crypto>.c file should just return an error!

This revision now requires changes to proceed.Aug 9 2017, 1:27 PM
jvijtiuk updated this revision to Diff 3.EditedAug 10 2017, 3:54 PM
  • Remove HAVE_DSA ifdefs where possible
  • Remove HAVE_DSA checks from public headers, libssh.h and server.h
  • Remove HAVE_DSA from private headers and most .c files.
  • Instead of checking for HAVE_DSA, handle DSA similarly to how ECC is handled, by failing when dsa keys are used with mbedtls.
  • Add dsa elements to structs which use dsa as void pointers, when mbedtls is used.

    Should the torture files, and tests in general check for HAVE_DSA so that DSA tests aren't run when libssh is built with mbedTLS at all? That is what happens right now. The alternative i can think of is that instead of not running any DSA tests, the tests except libssh to fail when DSA is used with mbedTLS.
asn added a comment.Aug 17 2017, 9:38 AM

All tests need to pass, so code which will not pass with the crypto lib need to be excluded, code commented out with #ifdef.

I need to talk to Aris, maybe we should move the DSA tests to its own file.

asn added a reviewer: aris.Aug 17 2017, 9:40 AM
asn changed the visibility from "All Users" to "Public (No Login Required)".Aug 17 2017, 3:02 PM
asn added a comment.Sep 11 2017, 5:20 PM

I've looked at the code again. It looks very clean and nice so far. Please address the issues I commented inline I will do a full review once I'm back from vacation in October.

src/bind.c
242

This does not change the code. Please remove the hunk.

src/ecdh_mbedcrypto.c
160

New to our coding conventions :-)

All pointer variables MUST be initialized to NULL. History has demonstrated that uninitialized pointer variables have lead to various bugs and security issues.

Pointers MUST be initialized even if the assignment directly follows the declaration, like q_c_string below, because the instructions sequence may change over time.

src/pki.c
939

This does not change the code. Please remove the hunk.

1002

This does not change the code. Please remove the hunk.

1602

This does not change the code. Please remove the hunk.

jvijtiuk updated this revision to Diff 22.Sep 12 2017, 2:52 PM

Remove formatting errors.
Initialize uninitialized pointers to null in mbedTLS files.

jvijtiuk updated this revision to Diff 23.Sep 12 2017, 3:18 PM

Remove formatting errors/hunks.
Remove unnecessary HAVE_LIBMBEDCRYPTO DSA check in pki_crypto.c

jvijtiuk updated this revision to Diff 24.Sep 12 2017, 3:30 PM

Add accidentaly removed o in mbedtls_info, introduced in revision 22.

asn requested changes to this revision.Oct 29 2017, 12:12 PM

Hello,

the patchset still needs several fixes especially for memory leak in error conditions. See inline comments for this!

Please make sure that all .c files you add have #include "config.h" at the very beginning! Rebase the patchset on master!

Please take a look at e99246246b4061f7e71463f8806b9dcad65affa0 and check if your lib needs something like that! I think it has the same problem, at least I do not see pthread_at_fork() being set up in mbettls. We do not want to have another security release ;)

As soon as those things are fixed I will merge it. I hope I spotted all the issues.

Andreas
src/ecdh_mbedcrypto.c
93

Please remove this if-statement, ssh_string_free can deal with NULL pointers.

172

Set ssh_set_error_oom(session) before the return?

src/libmbedcrypto.c
61

Missing SAFE_FREE(ctx);

66

Missing SAFE_FREE(ctx);

141

Missing SAFE_FREE(ctx);

146

Missing SAFE_FREE(ctx);

185

Missing SAFE_FREE(ctx);

190

Missing SAFE_FREE(ctx);

237

Missing SAFE_FREE(ctx);

242

Missing SAFE_FREE(ctx);

288

Missing SAFE_FREE(ctx);

293

Missing SAFE_FREE(ctx);

339

Missing SAFE_FREE(ctx);

344

Missing SAFE_FREE(ctx);

878

Remove the if-statement, SAFE_FREE takes care of checking for NULL pointer.

src/mbedcrypto_missing.c
59

Please use buf == NULL

src/options.c
1559

The 'case SSH_KEYTYPE_DSS' needs to be outside of this #ifdef!

src/pki_mbedcrypto.c
3

Remove leftover from conflict ... :-)

213

Please use if (rsa != NULL) however SAFE_FREE takes care of checking for a NULL pointer, not sure about the mbettls function.

217

Please use if (ecdsa != NULL) however SAFE_FREE takes care of checking for a NULL pointer, not sure about the mbettls function.

288

This should be goto fail or we leak memory from ssh_key_new().

354

This should be goto fail or we leak memory from ssh_key_new().

381

Use goto fail?

461

Please add a TODO here. Also we need to document this, this means that a libssh server is not able to deal with ECDSA keys. This function is essential to verify publickey authentication request.

Please create a README.mbedtls and explain this.

This revision now requires changes to proceed.Oct 29 2017, 12:12 PM
jvijtiuk updated this revision to Diff 31.EditedNov 17 2017, 6:14 PM

Hello,

I've fixed the memory leaks and config.h issues. README.mbedtls has also been added.
However, maybe the comparison code for ECDSA could be written with what mbedTLS already provides, the private and public curve points and the group can be accessed through the ECDSA context, however a function similar to EC_POINT_cmp doesn't exist, so I will have to look further into that when I find some time as I'm not sure what effect the curve group has on point comparison. The gcrypt code just compares the private and public points.

Regarding the multithreading issue with RNGs, so far the code uses only one source of entropy and one RNG for all the threads. The mbedTLS documentation reccomends only one source of entropy, and offers the possibiliy of using one or multiple RNGs. However it doesn't say how security could be affected when one RNG is shared between threads.

Since those two issues are still unsolved, i didn't rebase on master yet.

asn added a comment.Dec 1 2017, 9:05 AM
In D1#197, @jvijtiuk wrote:

Hello,

Hey Juraj,

I've fixed the memory leaks and config.h issues. README.mbedtls has also been added.

Thanks, that looks fine now. There is probably another issues. When checking known_hosts and in includes ecdsa keys you're not able to use them against the server because we can't compare the public key to the one the server sent us :(

However, maybe the comparison code for ECDSA could be written with what mbedTLS already provides, the private and public curve points and the group can be accessed through the ECDSA context, however a function similar to EC_POINT_cmp doesn't exist, so I will have to look further into that when I find some time as I'm not sure what effect the curve group has on point comparison. The gcrypt code just compares the private and public points.

If you document the limitation we could push this upstream as soon as the reseed issue is fixed. Then you could look into ecdsa.

Regarding the multithreading issue with RNGs, so far the code uses only one source of entropy and one RNG for all the threads. The mbedTLS documentation reccomends only one source of entropy, and offers the possibiliy of using one or multiple RNGs. However it doesn't say how security could be affected when one RNG is shared between threads.

You need to call mbedtls_ctr_drbg_reseed() see programs/ssl/ssl_fork_server.c in the mbedtls sources.

Since those two issues are still unsolved, i didn't rebase on master yet.

OK. Thanks. Looking forward to the next patch :-)

jvijtiuk updated this revision to Diff 32.EditedDec 11 2017, 1:22 PM
  • Add mbedtls drbg reseeding
  • Update README.mbedtls with info for ssh_key_cmp

When checking known_hosts and in includes ecdsa keys you're not able to use them against the server because we can't compare the public key to the one the server sent us :(

I've checked known_hosts.c, in the function check_public_key the keys are compared with memcmp on line 279. If i understood the code correctly, then mbedTLS not having ECDSA comparison support won't be a problem for checking known_hosts. However, i've documented in the README that whenever ssh_key_cmp is used, ECDSA comparison won't work.

You need to call mbedtls_ctr_drbg_reseed() see programs/ssl/ssl_fork_server.c in the mbedtls sources.

I've added the call to ssh_reseed to ssh_reseed.
Thanks for the help, I was tired and somehow misunderstood that the problem appeared with threads and not processes last time.

There is also a typo in the previous commit, the message states that a TODO for ed25519 was added instead of ECDSA.
I've tried to find how to edit commit messages with arcanist, however I couldn't find anything. However, apparently arc erases all commits during the review of the patch and commits the whole revision with the first revision message, so the typo won't be in the commit message in that case.

Sorry for any arc workflow mistakes I made, I still find it somewhat confusing at times.

I'll rebase on master and start working on ECDSA comparison if the known_hosts documentation is ok now.

asn added a comment.Dec 12 2017, 9:37 AM

This looks fine now. However it needs rebasing on master!

jvijtiuk updated this revision to Diff 33.Dec 21 2017, 1:39 PM

Rebased onto master and fixed conflicts that appeared.

Fixed a bug that was revealed when fixing conflicts, where nistp256 was always used for ECDH when using mbedTLS, even if other curves were selected.
Removed make_ecpoint hack from src/pki_mbedcrypto.c, since mbedtls_ecp_point_write_binary actually has a way of signaling whether the destination buffer is too small.

asn added a comment.Dec 21 2017, 3:13 PM

I've just commited a bigger patchset upstream. I think you need to rebase again. Sorry :-)

jvijtiuk updated this revision to Diff 34.EditedWed, Dec 27, 4:24 PM

I have rebased onto master again, and fixed the new conflicts that appeared.

asn accepted this revision.Thu, Dec 28, 11:15 AM

I think we are good to go. Thanks for your contributions!

This revision is now accepted and ready to land.Thu, Dec 28, 11:15 AM
Closed by commit rLIBSSHfd4e7b9b42df: add mbedtls crypto support (authored by jvijtiuk, committed by asn). · Explain Why
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.