ECDSA support for mbed TLS by katzer · Pull Request #385 · libssh2/libssh2

katzer · 2019-07-02T15:04:57Z

About

This PR adds support for ECDSA for both key exchange and host key algorithms.

The following elliptic curves are supported:

256-bit curve defined by FIPS 186-4 and SEC1
384-bit curve defined by FIPS 186-4 and SEC1
521-bit curve defined by FIPS 186-4 and SEC1

katzer · 2019-07-16T15:49:34Z

@willco007 Any comments?

tiennou · 2019-07-17T11:27:30Z

I'm poking at this using the WIP testcase PR (I have an ECDSA userauth testcase coming up).

Right now it's causing memory corruption, as it seems you're missing a call to mbedtls_ecdsa_init before calling mbedtls_ecdsa_genkey. With that fixed, mbedTLS complains about "PK - invalid key tag or value" when trying to parse the key, as it doesn't seem to know about OpenSSH-type keys — as per pk_parse.c.

AFAIU all our key support matrix issues are caused by our hostkey support going like this :

asks crypto backend to parse PEM/read DER (via init/initPEM).
crypto backend might use its own parser, or it might use our PEM parser (mbedTLS does the former, OpenSSL does the latter).

Hence, you might be able to crack open the key using our PEM parser, and use the things inside to build the key manually. This SO question seems to imply it's doable.

katzer · 2019-07-20T15:57:24Z

@tiennou

I'm poking at this using the WIP testcase PR (I have an ECDSA userauth testcase coming up).

👍

Right now it's causing memory corruption, as it seems you're missing a call to mbedtls_ecdsa_init before calling mbedtls_ecdsa_genkey.

Strange, for me it works. But in general you're right. It's missing there. I'll add it.

mbedTLS complains about "PK - invalid key tag or value" when trying to parse the key, as it doesn't seem to know about OpenSSH-type keys

Only PEM/DER format is supported. I successfully tested userauth and remote command execution with a ecdsa key created as follows:

ssh-keygen -m PEM -t ecdsa -b 256 -q -f $HOME/.ssh/dev.key -N ""

That creates a EC PRIVATE KEY. I dont get your point why to use another PEM parser?

tiennou · 2019-07-20T17:10:57Z

I did ssh-keygen -t ecdsa -f key_ecdsa, and got this key out (OpenSSH_7.9p1, LibreSSL 2.7.3, on macOS 10.14.4).

That creates a EC PRIVATE KEY. I dont get your point why to use another PEM parser?

Because the one provided by mbedTLS doesn't handle OpenSSH keys (like the one I generated above), so you're leaving that one the table compatibility-wise. Would you be so kind to PR my repo with a couple of those EC PRIVATE KEY so I can add them to the testsuite?

Hence, we have "a bunch" of pem parsers, one which knows RSA/DSA, one which knows OpenSSH. This one is used by Libgcrypt and WinCNG. OpenSSL uses our OpenSSH parser, but its own for RSA/DSA. mbedTLS uses its own parser exclusively.

Do note that I'm trying to grasp how the codebase is architected at the moment, so take what I'm saying with a grain a salt, but I feel that the mbedTLS backend should follow the same logic and go through our parser as well. And then add EC PRIVATE KEY support to our parser, so that all backends can benefit. I have a branch trying to make the parser architecture "saner", but it's not ready yet, so I can't provide a baseline…

Arguably, I was misled by the format issue, so if I get one of those keys, I'll give it a try and review. It's one less missing feature after all 😉.

katzer · 2019-07-30T13:37:08Z

@tiennou @willco007 I've also added support to handle OpenSSL keys. Would be nice if one of you could review my PR.

willco007 · 2019-07-30T22:59:15Z

@katzer the _libssh2_mbedtls_parse_openssh_key function only handles ecdsa style keys, I assume that is just because this is a WIP. Otherwise the parsing/building of the key type looks fine to me. 👍

tiennou

A few minor review comments, please take the macro stuff with a grain of salt.

AFAICT the mbedTLS backend never supported OpenSSH keys (OPENSSH PRIVATE KEY), the current architecture makes it hard to have consistent support for stuff, so I'm fine with it only supporting ECDSA keys for now.

src/mbedtls.c

katzer · 2019-07-31T09:01:40Z

@tiennou

A few minor review comments, please take the macro stuff with a grain of salt.

Done

AFAICT the mbedTLS backend never supported OpenSSH keys (OPENSSH PRIVATE KEY) [...] so I'm fine with it only supporting ECDSA keys for now.

Now I've implemented it, so lets keep it.

katzer · 2019-08-21T08:42:07Z

@willco007 @tiennou Any chance to see that being merged into master any time soon?

stale · 2019-12-25T14:11:46Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

src/mbedtls.c

willco007 · 2020-04-06T17:25:45Z

What state are we in with this, it looks ready to be merged, is that correct?

katzer · 2020-04-15T07:17:14Z

@willco007 Yes

willco007

I've added a couple comments about additional bounds checking, but otherwise it looks great. Thanks for the PR!

src/mbedtls.c

willco007 · 2020-05-15T17:41:45Z

src/mbedtls.c

+    p += 4;
+    *p = 0;
+
+    mbedtls_mpi_write_binary(mpi, p + 1, bytes - 1);


also advancing p here, and if bytes is 0 length this could go negative. Same below in the memmove.

src/mbedtls.c

willco007 · 2020-07-01T18:48:19Z

This PR is still waiting on bounds checks updates I noted on May 15, thanks.

..

Files: mbedtls.c, mbedtls.h, .travis.yml Notes: This PR adds support for ECDSA for both key exchange and host key algorithms. The following elliptic curves are supported: 256-bit curve defined by FIPS 186-4 and SEC1 384-bit curve defined by FIPS 186-4 and SEC1 521-bit curve defined by FIPS 186-4 and SEC1 Credit: Sebastián Katzer

This is just a stub to make `_libssh2_mbedtls_ecdsa_new_private` compile. mbedtls 3.6.0 silently deleted its public API `mbedtls_pk_load_file`, which this function relies on. Closes #1349

katzer changed the title ~~ECDSA support for mbed TLS~~ ECDSA support for mbed TLS - HELP NEEDED !!! Jul 2, 2019

katzer changed the title ~~ECDSA support for mbed TLS - HELP NEEDED !!!~~ ECDSA support for mbedTLS - HELP NEEDED !!! Jul 2, 2019

katzer changed the title ~~ECDSA support for mbedTLS - HELP NEEDED !!!~~ ECDSA support for mbed TLS Jul 2, 2019

katzer force-pushed the feature/mbedtls-ecdsa-support branch 7 times, most recently from 18a711f to 37efb24 Compare July 3, 2019 11:28

katzer marked this pull request as ready for review July 3, 2019 11:28

katzer force-pushed the feature/mbedtls-ecdsa-support branch 2 times, most recently from 4bc1ff2 to aa6e63e Compare July 4, 2019 09:51

katzer force-pushed the feature/mbedtls-ecdsa-support branch from aa6e63e to 668e783 Compare July 20, 2019 16:17

katzer force-pushed the feature/mbedtls-ecdsa-support branch 2 times, most recently from 97e12d9 to cda14b2 Compare July 30, 2019 15:10

tiennou reviewed Jul 31, 2019

View reviewed changes

src/mbedtls.c Outdated Show resolved Hide resolved

src/mbedtls.c Outdated Show resolved Hide resolved

src/mbedtls.c Outdated Show resolved Hide resolved

katzer force-pushed the feature/mbedtls-ecdsa-support branch 2 times, most recently from 6225a44 to 75320e9 Compare July 31, 2019 09:47

katzer force-pushed the feature/mbedtls-ecdsa-support branch from 8085158 to 2b405c3 Compare August 21, 2019 08:40

katzer force-pushed the feature/mbedtls-ecdsa-support branch from 2b405c3 to 6bc4b0b Compare September 26, 2019 14:00

slydder101 mentioned this pull request Feb 4, 2020

Deadlock on 32-bit Big-Endian ARM platforms in multi-threaded applications #447

Closed

willco007 reviewed Feb 5, 2020

View reviewed changes

src/mbedtls.c Outdated Show resolved Hide resolved

willco007 reviewed Feb 5, 2020

View reviewed changes

src/mbedtls.c Show resolved Hide resolved

katzer force-pushed the feature/mbedtls-ecdsa-support branch from 0490bfa to 882b8b2 Compare March 17, 2020 17:41

willco007 requested changes May 15, 2020

View reviewed changes

mrpippy reviewed May 16, 2020

View reviewed changes

src/mbedtls.c Outdated Show resolved Hide resolved

katzer force-pushed the feature/mbedtls-ecdsa-support branch 2 times, most recently from b82ae61 to 306a519 Compare June 2, 2020 05:49

katzer added 8 commits July 11, 2020 11:39

ecdsa support for mbedtls

5cda5be

Fix missing call to mbedtls_ecdsa_init

bff17e7

Parse openssh pem keys

0ef0207

cr fixes

448cf75

..

Fix a memory leak in _libssh2_mbedtls_ecdh_gen_k

191b46a

Fix minor formatting issue

fad41aa

Fix missing alloc check before using pubkey_oct

bcbda19

Inline hshlen variable

d9f4509

katzer force-pushed the feature/mbedtls-ecdsa-support branch from 306a519 to d9f4509 Compare July 11, 2020 09:39

Add checks before advancing the pointer

77c11f2

katzer requested a review from willco007 July 12, 2020 09:57

Fix segfault

fe749e9

willco007 merged commit 5528f3d into libssh2:master Sep 1, 2020

iamed2 mentioned this pull request Jan 25, 2021

SSH host verification fails when IP changes (git succeeds) JuliaLang/Pkg.jl#2334

Closed

sbredahl mentioned this pull request Apr 5, 2021

Add support for ECDSA signatures with MbedTLS backend #573

Closed

katzer deleted the feature/mbedtls-ecdsa-support branch January 5, 2026 14:33

Conversation

katzer commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

About

Uh oh!

katzer commented Jul 16, 2019

Uh oh!

tiennou commented Jul 17, 2019

Uh oh!

katzer commented Jul 20, 2019

Uh oh!

tiennou commented Jul 20, 2019

Uh oh!

katzer commented Jul 30, 2019

Uh oh!

willco007 commented Jul 30, 2019

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

katzer commented Jul 31, 2019

Uh oh!

katzer commented Aug 21, 2019

Uh oh!

stale bot commented Dec 25, 2019

Uh oh!

Uh oh!

Uh oh!

willco007 commented Apr 6, 2020

Uh oh!

katzer commented Apr 15, 2020

Uh oh!

willco007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willco007 May 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

willco007 commented Jul 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

katzer commented Jul 2, 2019 •

edited

Loading