Fix lifetimes in ssl::select_next_proto

[sfackler]

SSL_select_next_proto can return a pointer into either the client or server buffers, but the type signature of the function previously only bound the output buffer to the client buffer. This can result in a UAF in situations where the server slice does not point to a long-lived allocation.

Thanks to Matt Mastracci for reporting this issue.

[coolreader18]

In the advisory, this is listed as a non-vulnerable use of the API:

let server_protos = b"\x02h2".to_vec();
builder.set_alpn_select_callback(|_, client_protos| {
    ssl::select_next_proto(&server_protos, client_protos).ok_or_else(AlpnError::NOACK)
});

But that code no longer compiles with this patch, and I'm not exactly sure what the solution would be, besides to just leak server_protos (which isn't much of a solution at all). I feel like the main solution to this would be allowing borrows from the captured closure environment, but that isn't possible. Perhaps this could instead be a trait that allows borrowing from self, with a default implementation for closures to maintain backcompat? And then there could be a provided implementation that just uses select_next_proto, since I'd guess that's 99% of use cases.

[sfackler]

besides to just leak server_protos (which isn't much of a solution at all)

Unless you're dynamically constructing a bunch of short-lived contexts with different ALPN configurations that probably is a solution?

You could also write a select_next_proto equivalent in Rust that always returns a borrow out of client_protos. To be honest, it's wild to me that the OpenSSL API doesn't do that.

[coolreader18]

Unless you're dynamically constructing a bunch of short-lived contexts with different ALPN configurations that probably is a solution?

Fair enough lol. This is for rustpython, which exposes a _set_alpn_protocols method, so theoretically a user (or existing python code, or whatever) could indeed construct a bunch of contexts to do that. The second solution seems like a good idea, though it would be nice to not have to reimplement alpn parsing.

[coolreader18]

I guess this kinda thing just works, I think:

let proto =
    ssl::select_next_proto(&server, client).ok_or(ssl::AlpnError::NOACK)?;
let pos = memchr::memmem::find(client, proto)
    .expect("selected alpn proto should be present in client protos");
Ok(&client[pos..proto.len()])

[sfackler]

Yeah that's an easy brute-force way to fix it.