xous-core icon indicating copy to clipboard operation
xous-core copied to clipboard

Published 20 hours ago • verified publisher icon betrusted-io

`ring` 0.17 - monitor source crate for warning fixes

Open bunnie opened this issue 1 year ago • 4 comments

ring is now at 0.17, which means we could absorb this if it benefited other efforts in Xous.

TL;DR

To facilitate any testing, you may fetch the following branches:

  • https://github.com/betrusted-io/xous-core/tree/ring-0.17
  • https://github.com/betrusted-io/ring-xous/tree/0.17.7-merge

You will need to make sure that the top level Cargo.toml patch for ring points to wherever you cloned ring-xous, and then you can build a test image with:

cargo xtask app-image --feature simple-tls

@nworbnhoj : lib/tls breaks with the latest rustls API at 0.22.2. Could you please have a look at the API differences and let me know what the estimated effort level would be to upgrade the library?

@kotval / @nworbnhoj : moving forward on this depends heavily on if anyone finds their efforts on building Chat clients are held up on our version of ring being pinned at 0.16. If either of you say "this would greatly improve the porting process and reduce the effort to make it happen and/or make the chat clients easier to maintain", then, that would be a significant argument to move this forward sooner than later.

However, absent any actual demand to move the ring API forward, and given that such a forward movement would break some existing libraries, I don't see a compelling reason to finish the forward-port?

Details

I took a pass at bringing ring-xous to 0.17.7, which can be found here:

https://github.com/betrusted-io/ring-xous/tree/0.17.7-merge

The port can do TLS, but there are some problems that prevent this from being merged and moving this forward really depends upon how much we need the latest rustls. In particular:

  • The tls library that @nworbnhoj implemented breaks when you upgrade to the latest rustls. I don't know at what point things broke, but it looks like they did a major refactor in how they handle locally sourced certificates. It's not a "shallow" refactor of API names shuffling around -- looks like they took a good solid whack at that process. This means the stuff in the danger module needs a deep cut. I don't know how it all works, so I'd appreciate it if @nworbnhoj could have a look and render an opinion on the effort level.
  • There are warnings when compiling the latest ring-xous. The warnings look to actually be structural problems in ring:

First, they are using a deprecated method for doing prefixed exports:

warning: use of deprecated macro `prefixed_export`: `#[export_name]` creates problems and we will stop doing it.
   --> F:\code\ring-xous\src\arithmetic\montgomery.rs:136:1
    |
136 | prefixed_export! {
    | ^^^^^^^^^^^^^^^
    |
    = note: `#[warn(deprecated)]` on by default

warning: unused attribute `allow`
   --> F:\code\ring-xous\src\arithmetic\montgomery.rs:135:1
    |
135 | #[allow(deprecated)]
    | ^^^^^^^^^^^^^^^^^^^^
    |
note: the built-in attribute `allow` will be ignored, since it's applied to the macro invocation `prefixed_export`
   --> F:\code\ring-xous\src\arithmetic\montgomery.rs:136:1
    |
136 | prefixed_export! {
    | ^^^^^^^^^^^^^^^
    = note: `#[warn(unused_attributes)]` on by default

The maintainer has a TODO to stop doing this, but it is still pending even with the latest code:

https://github.com/briansmith/ring/blob/6762d656742d7ea431948356760da28e09292b72/src/arithmetic/montgomery.rs#L134-L136

Second, one function declaration is inconsistent:

warning: `CRYPTO_memcmp` redeclared with a different signature
   --> F:\code\ring-xous\src\c2rust\curve25519.rs:6:5
    |
6   | /     fn CRYPTO_memcmp(
7   | |         a: *const core::ffi::c_void,
8   | |         b: *const core::ffi::c_void,
9   | |         len: size_t,
10  | |     ) -> core::ffi::c_int;
    | |_________________________^ this signature doesn't match the previous declaration
    |
   ::: F:\code\ring-xous\src\prefixed.rs:120:9
    |
120 |           #[$attr = $prefixed_name]
    |           ------------------------- `CRYPTO_memcmp` previously declared here
    |
    = note: expected `unsafe extern "C" fn(*const u8, *const u8, usize) -> i32`
               found `unsafe extern "C" fn(*const c_void, *const c_void, u32) -> i32`
    = note: `#[warn(clashing_extern_declarations)]` on by default

The actual implementation is here:

https://github.com/briansmith/ring/blob/6762d656742d7ea431948356760da28e09292b72/crypto/mem.c#L60

And the declaration is here:

https://github.com/briansmith/ring/blob/6762d656742d7ea431948356760da28e09292b72/src/constant_time.rs#L34-L36

The declaration is an actual mismatch with the code in the maintainer's upstream. I think maybe it doesn't create a warning on most targets because this function is monkey-patched to an assembly implementation on most platforms. In fact, in general, a really awful design pattern has crept into ring, where ring uses Rust FFI declarations in one name space, and then they use a series of macros to switch the names to an implementation-dependent, version-dependent external symbol that gets linked into FFI object files; so it really works hard to throw away any semblance of safety guarantees across FFI interfaces that Rust could provide. I guess they are "just really careful" in implementing this stuff.

Anyways, this is fixable with a patch to the code -- in practice, I think the mismatch doesn't break anything? but I'd have to think about it.

Because the tls lib is broken on the latest rustls, to do testing I re-introduced the rustls direct test stub in this branch, which you can build using this command line:

cargo xtask app-image --feature simple-tls

I am able to do the most trivial TLS handshake with this:

INFO:shellchat::cmds::net_cmd: connect TCPstream to bunniefoo.com (services\shellchat\src\cmds\net_cmd.rs:581)
INFO:shellchat::cmds::net_cmd: create http headers and write to server (services\shellchat\src\cmds\net_cmd.rs:584)
TRCE:rustls::client::hs: We got ServerHello ServerHelloPayload {
                                                                    legacy_version: TLSv1_2,
                                                                                                random: a54b32c11dd88da42d201fa42e048764c9a5b91c5b4ba2939821867b9f7698e2,
                                                                                                                                                                             session_id: ,
           cipher_suite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
                                                                   compression_method: Null,
                                                                                                extensions: [
                                                                                                                     RenegotiationInfo(
                                                                                                                                                   ,
                                                                                                                                                            ),
                                                                                                                                                                      EcPointFormats(
              [
                               Uncompressed,
                                                            ANSIX962CompressedPrime,
                                                                                                    ANSIX962CompressedChar2,
                                                                                                                                        ],
                                                                                                                                                  ),
                                                                                                                                                            SessionTicketAck,
                                                                                                                                                                                 ],
                                                                                                                                                                                   } (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\hs.rs:483)
DBG :rustls::client::hs: ALPN protocol is None (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\hs.rs:469)
DBG :rustls::client::hs: Using ciphersuite TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\hs.rs:605)
DBG :rustls::client::tls12::server_hello: Server supports tickets (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\tls12.rs:90)
DBG :rustls::client::tls12: ECDHE curve is EcParameters { curve_type: NamedCurve, named_group: secp256r1 } (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\tls12.rs:396)
TRCE:rustls::client::tls12: Server cert is CertificateChain([CertificateDer(0x3082063c30820524a003020102021100ac17f9376e7b0b396b223fb9031b04f2300d06092a864886f70d01010b050030818f310b3009060355040613024742311b30190603550408131247726561746572204d616e636865737465723110300e0603550407130753616c666f726431183016060355040a130f5365637469676f204c696d69746564313730350603550403132e5365637469676f2052534120446f6d61696e2056616c69646174696f6e2053656375726520536572766572204341301e170d3233313231343030303030305a170d3235303131333233353935395a301c311a3018060355040313117777772e62756e6e6965666f6f2e636f6d30820122300d06092a864886f70d01010105000382010f003082010a0282010100bce62b07a661f857c32b63bc9f828f6ae05e8dc6e0f5215d6185aab00885467829208bfed406b6bb9857efbb12edc8f327f543218456f9384e246147dffb6fdbc3ea292d141fbac856921882893ba23097161d94b80adc175ee2942c20023f8a6234603bed5137994b187c278040e277a0e5310e2dcdfb536ad5ed89b0a62b19cc05dd46b83bf07a7c4b6d7227247b90d0aaf3eea6bcc9f139906f5ed1975b8137352ea44cd9d72bc54dc8ea704dcc92198ddc993fc8873d45536349d5bf15c25be1f398b9b5de1a5a29c1fbb8fbf14589edd024ce4f89d934a9fc5380fb1bbcf0192961b7c7208f8bfba803c56295f2cea36ba9ff0d25cd9c2e279232269ef30203010001a3820303308202ff301f0603551d230418301680148d8c5ec454ad8ae177e99bf99b05e1b8018d61e1301d0603551d0e04160414b101a290ff9eb114112884b28a9ab5e7f1c72ce7300e0603551d0f0101ff0404030205a0300c0603551d130101ff04023000301d0603551d250416301406082b0601050507030106082b0601050507030230490603551d20044230403034060b2b06010401b231010202073025302306082b06010505070201161768747470733a2f2f7365637469676f2e636f6d2f4350533008060667810c01020130818406082b0601050507010104783076304f06082b060105050730028643687474703a2f2f6372742e7365637469676f2e636f6d2f5365637469676f525341446f6d61696e56616c69646174696f6e53656375726553657276657243412e637274302306082b060105050730018617687474703a2f2f6f6373702e7365637469676f2e636f6d302b0603551d110424302282117777772e62756e6e6965666f6f2e636f6d820d62756e6e6965666f6f2e636f6d3082017f060a2b06010401d6790204020482016f0482016b0169007600cf1156eed52e7caff3875bd9692e9be91a71674ab017ecac01d25b77cecc3b080000018c6a0dc75a0000040300473045022100f2aeade2cb8e5ebe45c4f236ba98677159565766ddf8b57bd4135677b4b2cf860220115685412aaaddc26b3c06d3d1541c2c98babc61be9c426304dfc0a4b2799019007700a2e30ae445efbdad9b7e38ed47677753d7825b8494d72b5e1b2cc4b950a447e70000018c6a0dc7d40000040300483046022100923701646bbe572f18c0335f842ab97fd633e221cb8d1a8a6f8807a8760c20d6022100adde31a95b558fb74cd17e6004f217c360bf0cb692e2ebe8fa62f2a8d5d103db0076004e75a3275c9a10c3385b6cd4df3f52eb1df0e08e1b8d69c0b1fa64b1629a39df0000018c6a0dc7360000040300473045022100f2c4e95a3bd7c2232e3662811fc5458e3e4134e0bd3d346c77b2904d1b57dab5022018a40865be901c44b4cba554f98ec891826383b84c32dc3466d12a9705943930300d06092a864886f70d01010b05000382010100522d7f4148ebc3742aba0fa4bc3704b04eefbe8fac7c27cc72da0c28b2b9d7d34278fc4031a906a171fd0dd0446e8c1572935c63fadba7f96f7204ccf22f813b41df40b75b0aae2cd6a426c232a0ef2538f82ded198812a46dcfaa295a0976ac2c1330fbb52f8580072d42f54e5f520a68ee7ea5ccc2d6061f59e3296073d45c39d120be20517562c534c8a49e6fef443e004dc36d9da9e99c7de73ff7d3d81e8317b37917c02d7a7be6b86d12641c6efdea9c3dbdf4c5b33bef42d10061e7bf3afb062aa9bdaf5def66249ba8b7efac5cb7f3c43a5280aea7eac8eea868babccd98239698e3201de15a2bb8da374b88572f1011190fd0689ab38370ce5d38fe), CertificateDer(0x30820613308203fba00302010202107d5b5126b476ba11db74160bbc530da7300d06092a864886f70d01010c0500308188310b3009060355040613025553311330110603550408130a4e6577204a6572736579311430120603550407130b4a65727365792043697479311e301c060355040a131554686520555345525452555354204e6574776f726b312e302c06035504031325555345525472757374205253412043657274696669636174696f6e20417574686f72697479301e170d3138313130323030303030305a170d3330313233313233353935395a30818f310b3009060355040613024742311b30190603550408131247726561746572204d616e636865737465723110300e060355040713075361 (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\tls12.rs:686)
DBG :rustls::client::tls12: Server DNS name is DnsName("bunniefoo.com") (C:\Users\bunnie\.cargo\registry\src\index.crates.io-6f17d22bba15001f\rustls-0.22.2\src\client\tls12.rs:687)
INFO:shellchat::cmds::net_cmd: readout cipher suite (services\shellchat\src\cmds\net_cmd.rs:596)
INFO:shellchat::cmds::net_cmd: Current ciphersuite: TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (services\shellchat\src\cmds\net_cmd.rs:598)
INFO:shellchat::cmds::net_cmd: read TLS response (services\shellchat\src\cmds\net_cmd.rs:600)
INFO:shellchat::cmds::net_cmd: len: 291 (services\shellchat\src\cmds\net_cmd.rs:602)
INFO:shellchat::cmds::net_cmd: HTTP/1.1 200 OK
Date: Sun, 11 Feb 2024 09:14:29 GMT
Server: Apache/2.4.10 (Debian)
Last-Modified: Mon, 05 Jun 2017 16:46:12 GMT
ETag: "2d-551393da14d9b"
Accept-Ranges: bytes
Content-Length: 45
Connection: close
Content-Type: text/html

<html>
        Ce n'est pas une page vide.
                                   </html>
                                           (services\shellchat\src\cmds\net_cmd.rs:603)

I would hesitate to say "it works" at this point, it's more like "nothing is obviously broken and preventing forward progress". However, I don't want to merge this because I'm not sure if it's worth it right now. To move ahead, I think we have to satisfy two prerequisites:

  • [x] lib/tls has to be ported to the rustls 0.22.2 API level
  • [x] we actually need a reason to do this work, i.e., some part of the chat client effort is blocked on ring 0.17

bunnie avatar Feb 11 '24 09:02 bunnie

Overall, the changes to rustls appear to be sound, and while there is a little work to upgrade xous/libc/tls, it seems manageable and sensible.

Some of the changes to rustls are a relatively straight-forward relocation of structs (ServerCertVerified & WebPkiVerifier) . The more impactful change is the abolition of rustls::Certificate and rustls::OwnedTrustAnchor in favor of rustls::pki_types::CertificateDer.

rustls v.21.7 utilised Certificate and OwnedTrustAnchor to hold only the critical X.509 fields. This was an attractive aspect for xous from a size/storage perspective. rustls v.22.2 uses a rustls::pki_types::CertificateDer in its operations, retaining the full X.509 structure.

The obvious (and favored) path forward is to similarly adopt rustls::pki_types::CertificateDer within xous/libc/tls and save this structure to the pddb. This is obviously a (relatively minor) breaking change - and will be (somewhat) less space efficient - but the end result should be more simple code.

The obvious alternative would be to define OwnedTrustAnchor in xous/libc/tls and retain the size advantages - but this also means introducing the various translations between rustls::pki_types::CertificateDer and OwnedTrustAnchor that rustls has chosen to avoid. This is likely an unwise choice.

I will progress along these lines and submit a PR - all going well.

nworbnhoj avatar Feb 14 '24 23:02 nworbnhoj

What's the size difference of holding a whole X.509 certificate versus just the critical fields? My intuition says it's on the order of 10's of kiB, so it shouldn't be a problem to retain the whole certificate. We just have to retain a handful of them too, correct? I did notice when I tried to compile against the static list of certificates from webpki crate that it add +2MiB to the binary size but I assumed we wouldn't carry that around and instead we'd just cherry pick a few certificates from that list.

Basically if the impact is on the order of 10kiB (or even low 100's kiB) I'm not too worried about the size impact. But if we are talking about +2MiB of static data being linked in I should take a closer look.

bunnie avatar Feb 15 '24 04:02 bunnie

Looks like the tls feature compiles, I'll go ahead and merge this now so we're at least at ring 0.17.

I'm going to change the title of the issue to reflect that the upgrade is done, but, we need to check in periodically on the ring development and see if the deprecation notices and warnings have been cleaned up from the code base...

bunnie avatar Mar 01 '24 17:03 bunnie

The new TODO:

  • [ ] clean up deprecation warnings, once they are cleaned up from the maintainer's crate
  • [ ] clean up inconsistent method declaration, once it's fixed in the maintainer's crate

I think unless something is urgently broken, this is a low-priority task, and we'll keep an eye on ring 0.17 and wait until all the warnings are cleaned up. I have a feeling that something awful might be in store for the current practice used of patching all the function calls, so fixing these remaining warnings could be quite a task.

bunnie avatar Mar 01 '24 17:03 bunnie