SSH clone fails against remotes running newer OpenSSH versions

[Keno]

libssh2 advertises support for diffie-hellman-group-exchange-sha256, but that kex protocol seems to be buggy or otherwise unsupported, causing Pkg clone failures if the remote has a decently up-to-date openssh. Right now GitHub does not have support for that protocol, but we should either patch libssh2 to disable this feature of figure out what's broken to avoid nasty surprises if GitHub upgrades.

[Keno]

Seems to only be an issue with the mbedTLS backend, cc @wildart

[Keno]

SSH log: https://gist.github.com/Keno/5052e76475ae56653ce398032e758404

[wildart]

I bet there is some flag that disables this protocol. Try to disable it with LIBSSH2_DH_GEX_NEW.

[Keno]

Ok, the solution is simple. The required numbers are larger than MBEDTLS_MPI_MAX_SIZE (2055 > 2048), so all random numbers are 0, and the server gets confused (and the client doesn't do error checking. We should bump MBEDTLS_MPI_MAX_SIZE (maybe to 3072). I have also started a PR to add proper error checking to libssh2 here: libssh2/libssh2#119.

[wildart]

make it, 4096

[wildart]

Have you seen this https://github.com/ARMmbed/mbedtls/blob/9803d07a63621697ff2f632066dce4d57f0f60fc/include/mbedtls/bignum.h#L70-L71?

[Keno]

Oh, hold on, there's an even worse bug. libssh2 passes the desired number of bits, but the mbedtls api takes bytes.

[wildart]

That could be my fault.

[Keno]

Well, many people took a look at that code, and nobody caught it so far, so I think there's some collective blame to go around. Let's fix it in any case.

[Keno]

I wonder if fixing that would allow us to not have to patch the config file

[wildart]

I see. I used mbedtls_mpi_size instead of mbedtls_mpi_bitlen in gen_publickey_from_rsa in mbedtls.c.
No, I was correct.

[Keno]

No, I think that's correct.

[Keno]

The problem is in _libssh2_bn_rand

[Keno]

What we probably need to do is:

int _libssh2_bn_rand(bn, bits, top, bottom) {
    err = mbedtls_mpi_fill_random(bn, bits/8+1, mbedtls_ctr_drbg_random, &_libssh2_mbedtls_ctr_drbg)
    // check err, etc
    // zero out top bits%8 bits
}

Do you want to take care of that? In that case I'll move on to something else.

[Keno]

Hey, look we're not the only ones who were confused about that parameter: libssh2/libssh2#103

[wildart]

I'll fix it. Why do you need to zero out top bits%8 bits?

[Keno]

Otherwise you get numbers that are too large by a couple of bits. I don't see a problem with that in the way the numbers are used by libssh2 (since g^x retains wrap-around properties), but I imagine it would be good to follow the API in any case?

[wildart]

Indeed, problem was in incorrect byte size, so no config patch required. I'll submit libssh2-mbedtls patch ASAP.