Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

networking: delete std.x; add std.crypto.tls and std.http.Client #13980

Merged
merged 59 commits into from
Jan 3, 2023

Conversation

andrewrk
Copy link
Member

@andrewrk andrewrk commented Dec 16, 2022

First, this branch deletes the std.x namespace. I originally agreed to have this "experimental namespace" for @lithdew to play around with networking. Well, playtime is over. I'm working on networking now. So anything valuable that is deleted from std.x needs to get properly upstreamed, by being reintroduced and undergoing code review that was not previously done on the std.x namespace. The null hypothesis is that it's all gone, starting now.

Next, this branch introduces TLS 1.3 support to the Zig standard library. So far, this implementation does not heap allocate under any conditions and it is my goal to keep it that way. This implementation is only just getting started and needs a lot of work until it is ready to be used:

Finally, this branch adds a very simple HTTP client in order to test drive the TLS implementation.

lib/std/crypto/tls.zig Outdated Show resolved Hide resolved
@Jarred-Sumner
Copy link
Contributor

Very exciting! A few things to look at for future macOS specific optimizations:

@leap0x7b
Copy link
Contributor

cool certificate bro

@ghost
Copy link

ghost commented Dec 17, 2022

I also find this very exciting, as I complained before about the haphazard organization of std.net vs std.x. I think at least these things should get upstreamed into std.net:

  • Reader and Writer structs on net.Stream that use the send and recv syscalls, allowing you to pass flags (currently you can only get Reader and Writer that call os.read and os.write, with no flags support)
  • A tagged union version of Address?
  • A function to parse a string into an address + port

@jorangreef
Copy link
Contributor

CC'ing @guidovranken — we were chatting only yesterday about how to get TLS into Zig (before I saw this!). It would be awesome if we could get this into https://github.com/guidovranken/cryptofuzz for fuzzing.

@andrewrk will this be able to be decoupled from network I/O? For example, so that you can treat it as a state machine to make testing easier, or to integrate with different I/O stacks?

@andrewrk
Copy link
Member Author

Thank you for the introduction. Hi @guidovranken - pleased to meet you. I would be very interested in working with you on getting this implementation properly fuzz tested... after it is ready of course. It is still in an exploratory phase currently.

@andrewrk will this be able to be decoupled from network I/O? For example, so that you can treat it as a state machine to make testing easier, or to integrate with different I/O stacks?

Yes. My current plan is to expose the API in the std.crypto.tls namespace and have it accept a "Context" object, similar to how spawning threads works, and how sorting works. Idea being that you pass in an instance of an object of any type as long as it has the following methods:

const Example = struct {
    // put any fields you want here

    pub const WriteError = error {
        // ...
    };

    pub fn writev(example: *Example, iovecs: []const std.os.iovec_const) WriteError!usize {
        // your implementation here...
    }

    pub const ReadError = error {
        // ...
    };

    pub fn read(example: *Example, buffer: []u8) ReadError!usize {
        // your implementation here...
    }
};

Usage would then look something like this:

var example: Example = .{
    // initialize your object
};
var tls_client = try std.crypto.tls.Client.init(&example, tls_options);
const amt_written = try tls_client.write(&example, bytes_to_send);
const amt_read = try tls_client.read(&example, buffer);

You can imagine how one might build a higher level API on top of this.

I would be interested in working with members of the TigerBeetle team to iterate on this API specifically with regards to integration with your IO_uring abstraction. It is my intention to make the API as simple as possible, while maintaining the property that it can be optimally integrated with any reasonable I/O stack - especially one based on IO_uring as a case study.

@guidovranken
Copy link

Sure, I'm now subscribed to this thread and I'll keep an eye on it.

OpenSSL and other projects have a very similar way of fuzzing the TLS stack. The nice thing about this is that the fuzzing corpora will be largely compatible, and we can take the corpus of OpenSSL and other TLS software and use it for the Zig TLS fuzzer, so we instantly have a lot of code coverage.

My Cryptofuzz project actually does not fuzz TLS functions, but only core cryptographic functions (including bignums). This has previously found a few bignum bugs in Zig. I integrated the Cryptofuzz Zig fuzzer into OSS-Fuzz the other day; I will send you any bugs it might find.

Currently the Zig harness for Cryptofuzz only supports bignum ops and HKDF: https://github.com/guidovranken/cryptofuzz/blob/master/modules/zig/cryptofuzz.zig

I work on Cryptofuzz whenever I have time (it's unpaid work apart from commissions). I would like to support more cryptographic primitives like hashing, encryption and elliptic curve ops. If anyone in the Zig ecosystem has time to implement this for Cryptofuzz I'd be happy to get in touch and work with them. This will speed up the development. It's well worth the time investment as the project has found hundreds of bugs in major cryptographic libraries.

Once the TLS library is ready, I propose to make a dedicated zig project on OSS-Fuzz, where we will put the TLS fuzzer, the Cryptofuzz Zig fuzzer, and other stdlib fuzzers.

Copy link
Contributor

@nektro nektro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch (@intToEnum(HandshakeType, handshake_type) { ?

@andrewrk
Copy link
Member Author

@jedisct1 do I understand correctly that std.crypto does not yet have rsa_pkcs1 implemented?

@jedisct1
Copy link
Contributor

Not yet, this is what I'm working on as we speak.

@andrewrk
Copy link
Member Author

Wonderful! I'll move on to other tasks besides certificate validation for now, then. I hope you are avoiding heap allocation in the implementation. We can always know the upper bound of calculations since the bit width is fixed (i.e. 1024, 2048, 4096). If you wanted to experiment with literally using i1024, i2048, i4096 integer types, let me know and I will start working on the compiler improvements to ensure that the required operations (multiplication, division, etc.) will be lowered properly on such large types. Currently, beyond i128, one will get a crash in LLVM lowering code for some operations.

For what it's worth, I noticed on jedisct1/libsodium#368 that it would be beneficial to try to avoid RSA. I tried asking google.com for a non-RSA signature like this:

--- a/lib/std/crypto/tls/Client.zig
+++ b/lib/std/crypto/tls/Client.zig
@@ -53,15 +53,9 @@ pub fn init(stream: net.Stream, ca_bundle: crypto.CertificateBundle, host: []con
         0x02, // byte length of supported versions
         0x03, 0x04, // TLS 1.3
     }) ++ tls.extension(.signature_algorithms, enum_array(tls.SignatureScheme, &.{
-        .rsa_pkcs1_sha256,
-        .rsa_pkcs1_sha384,
-        .rsa_pkcs1_sha512,
         .ecdsa_secp256r1_sha256,
         .ecdsa_secp384r1_sha384,
         .ecdsa_secp521r1_sha512,
-        .rsa_pss_rsae_sha256,
-        .rsa_pss_rsae_sha384,
-        .rsa_pss_rsae_sha512,
         .ed25519,
     })) ++ tls.extension(.supported_groups, enum_array(tls.NamedGroup, &.{

but it sent RSA-signed certificates regardless.

@jedisct1
Copy link
Contributor

jedisct1 commented Dec 21, 2022

@andrewrk RSA keys and signatures are big, it can be tricky to us and implement safely, and there are usually better alternatives.

However, for TLS certs, if someone decided to only sign their cert with RSA, there's not much we can do besides comply and use RSA to verify them.

RSA also still has some unique properties that are useful in modern protocols (for example, see blind RSA signatures that I'm an co-author of -- It has a Zig implementation that currently depends on OpenSSL/BoringSSL which is sad).

For native integers > 128 bits, jedisct1@36833db has been sitting in my tree for a while (which, at least, allows formatting large numbers), but I forgot to turn it into a PR.

@andrewrk andrewrk force-pushed the std.net branch 2 times, most recently from 134381b to 2e278f6 Compare December 23, 2022 07:15
@andrewrk andrewrk force-pushed the std.net branch 3 times, most recently from 0dc85f9 to 98de5c7 Compare December 30, 2022 02:00
Here's what I landed on for the TLS client. It's 16896 bytes
(max_ciphertext_record_len is 16640). I believe this is the theoretical
minimum size, give or take a few bytes.

These constraints are satisfied:
 * a call to the readvAdvanced() function makes at most one call to the
   underlying readv function
 * iovecs are provided by the API, and used by the implementation for
   underlying readv() calls to the socket
 * the theoretical minimum number of memcpy() calls are issued in all
   circumstances
 * decryption is only performed once for any given TLS record
 * large read buffers are fully exploited

This is accomplished by using the partial read buffer to storing both
cleartext and ciphertext.
 * fix eof logic
 * fix read logic
 * fix VecPut logic
 * add some debug prints to remove later
Before this, it incorrectly returned true when there was still cleartext
to be read.
This is a streaming HTTP header parser. All it currently does is detect
the end of headers. This will be a non-allocating parser where one can
bring supply their own buffer if they want to handle custom headers.

This commit also improves std.http.Client to not return the HTTP headers
with the read functions.
The code we are borrowing from https://github.com/shiguredo/tls13-zig
requires an Allocator for doing RSA certificate verification. As a
stopgap measure, this commit uses a FixedBufferAllocator to avoid heap
allocation for these functions.

Thank you to @naoki9911 for providing this great resource which has been
extremely helpful for me when working on this standard library TLS
implementation. Until Zig has std.crypto.rsa officially, we will borrow
this implementation of RSA. 🙏
This commit introduces tls.Decoder and then uses it in tls.Client. The
purpose is to make it difficult to introduce vulnerabilities in the
parsing code. With this abstraction in place, bugs in the TLS
implementation will trip checks in the decoder, regardless of the actual
length of packets sent by the other party, so that we can have
confidence when using ReleaseFast builds.
This commit adds `writeEnd` and `writeAllEnd` in order to send data and
also notify the server that there will be no more data written.

Unfortunately, it seems most TLS implementations in the wild get this
wrong and immediately close the socket when they see a close_notify,
rather than only ending the data stream on the application layer.
It appears to be implemented incorrectly in the wild and causes the read
connection to be closed even though that is a direct violation of RFC
8446 Section 6.1.

The writeEnd function variants are still there, ready to be used.
Previously, the code only checked Common Name, leading to unable to
validate valid certificates which relied on the subject_alt_name
extension for host name verification.

This commit also adds rsa_pss_rsae_* back to the signature algorithms
list in the ClientHello.
Although RFC 8446 states:

> Each party MUST send a "close_notify" alert before closing its write
> side of the connection

In practice many servers do not do this. Also in practice, truncation
attacks are thwarted at the application layer by comparing the amount of
bytes received with the amount expected via the HTTP headers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.