Can probably get rid of this, eh?

🧐

Whoops. Reverted.

I'd suggest using checked arithmetic throughout this module to avoid potential integer overflow issues, i.e.

&frame_copy[DATA_LEN_SIZE..(DATA_LEN_SIZE.checked_add(chunk_length).unwrap() as usize)]

Good point! I changed all constants which will mostly be used as usize to actually being usizes. So I changed the above to DATA_LEN_SIZE.checked_add(chunk_length as usize).unwrap(), where chunk_length is read from 4 bytes. So the inner cast should be fine.

Would suggest:

cnt = cnt.checked_add(1).unwrap();

I've deleted cnt because it wasn't used anywhere. But good to know!

It looks like this is an old version and that's what's causing it to pull in an old version of subtle. Can you bump this to 0.3? (the ^ is also unnecessary as Cargo is semver-by-default)

Thanks, updated x25519-dalek to 0.3 and randto0.5`. This still fails on stable.

It looks like x25519-dalek enables its nightly feature (and vicariously subtle's nightly feature) by default (which is probably a mistake/oversight):

https://github.com/dalek-cryptography/x25519-dalek/blob/2abfb37/Cargo.toml#L38

I think this will fix it:

x25519-dalek = { version = "0.3", default-features = false, features = ["std", "u64_backend"] }

Thanks, that fixes that indeed (at least locally it works now).

---- secret_connection::tests::test_incr_nonce stdout ----
	thread 'secret_connection::tests::test_incr_nonce' panicked at 'attempt to add with overflow', src/secret_connection.rs:356:9

I think this would be a lot clearer (and panic-free!) if you just kept an internal 64-bit counter and used BigEndian::write_u64 (from the byteorder crate) to serialize it.

That'd also make it easier to switch to little endian, if there's still time to do that 😉

Suggestion:

impl<IoHandler> io::Write for SecretConnection<IoHandler>
where
    IoHandler: : io::Read + io::Write + Send + Sync>
{

Would suggest checked arithmetic everywhere (really wish clippy had a lint for this):

let mut leftover_portion = vec![0; self.recv_buffer.len().checked_sub(n).unwrap()];

Suggestion:

impl<IoHandler> io::Read for SecretConnection<IoHandler>
where
    IoHandler: : io::Read + io::Write + Send + Sync
{

I think it'd be good to define an ErrorKind variant for MAC verification errors (perhaps VerificationError or CryptoError?) and use an Error here

Given opening_key and nonce both come from SecretConnection, any reason to not make this a private method on SecretConnection? It looks like a corresponding seal() method could be useful too.

You should be able to just use BigEndian::write_u32 here and avoid boxing chunk_length in an array

Suggest checked arithmetic:

n = n.checked_add(chunk.len()).unwrap();

I'd suggest replacing this with a u64 counter, and using BigEndian::write_u64 to serialize it. To fill a 96-bit nonce, you can fill the first 32-bits with zeroes, e.g.:

struct Nonce(pub [u8; 12]);

impl From<u64> for Nonce {
    fn from(counter: u64) -> Nonce {
        let mut nonce = [0u8; 12];
        BigEndian::write_u64(counter, &mut nonce[4..]);
        Nonce(nonce)
    }
}

Or if you're really into doing arithmetic on byte arrays, you could do it like this:

struct Nonce(pub [u8; 12]);

impl Default for Nonce {
    fn default() {
        Nonce([0u8; 12])
    }
}

impl Nonce {
    fn increment(&mut self) {
        let counter = BigEndian::read_u64(&self.0[4..]);
        BigEndian::write_u64(counter.checked_add(1).unwrap(), &mut self.0[4..]);
    }
}

Thanks!

I went with the 2nd approach but changed increment to behave exactly as the golang implementation (which makes another counter for the 1st 4 bytes necessary). If you think we should stick with 64 bits, I'll quickly change it. See 2e5d98f

I think it's a lot simpler to just use a 64-bit integer (and panic on overflow). The chances of overflowing one in practice actually sending network messages are practically nonexistent (and you should rekey long before that anyway).

OK, will change to 64 bits and update the tests.