Bouke / HAP

Swift implementation of the Homekit Accessory Protocol
https://boukehaarsma.nl/HAP/
MIT License
366 stars 50 forks source link

Cannot add >23 accessories to bridge #57

Closed Bouke closed 6 years ago

Bouke commented 6 years ago

Danny reached out to me by e-mail with an issue not being able to add >23 accessories, which I confirmed on my end as well.

The one with 23 items, the size is: 15,744 bytes

[hap.endpoints|INFO] 192.168.1.214 GET /accessories - 200 15744

The one with 24 items, the size is: 16,344 bytes

[hap.endpoints|INFO] 192.168.1.214 GET /accessories - 200 16344

The error showing up in the server's logs is:

[hap.http|ERROR] Error while reading from socket [ERROR: Error code: -9971(0x-26F3), Connection reset by peer]
dmavromatis commented 6 years ago

I've been able to reproduce having the error of, "Connection reset by peer" by passing in an incorrect length and then writing to the socket -- so, I'm still a bit suspicious of this line that calculates length:

let length = UInt16(data.count).bigEndian.bytes

When you hit the 16,385 byte length, the returned "length" is: 0140 hex or 320 dec which doesn't make sense to me -- but perhaps is correct as Big Endian?

... but it seems to be working fine at 16,384, the returned "length" is: 0040 hex or 64 dec and I don't see any 16k limits anywhere except maybe if the data count exceeds the size of UInt16 (UInt16(data.count)) -- but we are nowhere close to that given we're at 16k for the content length?

That said, I have confirmed and been able to reproduce the following issue related to 16k -- specifically when the writeBuffer is over 16,384 bytes issue...

This does not work, the WriteBuffer before encryption is called is 16,385, 1 byte over before :

[hap.endpoints|INFO] 192.168.1.214 GET /accessories - 200 16307
[hap.http|ERROR] WriteBuffer 16385
[hap.http|ERROR] Socket Writing 16403: nil
written...16403
[hap.http|ERROR] Error while reading from socket [ERROR: Error code: -9971(0x-26F3), Connection reset by peer]

This does not work, you can see the WriteBuffer before encryption is called is 16,388, 4 bytes over:

[hap.endpoints|INFO] 172.20.10.1 GET /accessories - 200 16310
WriteBuffer 16388: Optional("HTTP/1.1 200 OK\r\nContent-Type: application/hap+json\r\nContent-Length: 16310
[hap.http|ERROR] Socket Writing 16406: nil
written...16406
[hap.http|ERROR] Error while reading from socket [ERROR: Error code: -9971(0x-26F3), Connection reset by peer]

This, obviously, works as you can see the WriteBuffer before encryption is called is 16,161, well below the 16,384 bytes:

[hap.endpoints|INFO] 172.20.10.1 GET /accessories - 200 16083
WriteBuffer 16161: Optional("HTTP/1.1 200 OK\r\nContent-Type: application/hap+json\r\nContent-Length: 16083

This works too, the WriteBuffer before encryption is called is exactly 16,384 bytes:

[hap.endpoints|INFO] 192.168.1.214 GET /accessories - 200 16306
[hap.http|ERROR] WriteBuffer 16384
[hap.http|ERROR] Socket Writing 16406: nil
written...16406

Note, the Bluesocket is successfully writing the full amount over the 16,384 bytes as you can see in the last example, "written...16406". So I don't think the socket write is the issue.

Thus, I'm thinking the crypto is causing this issue with anything over 16,384 bytes?

Doesn't crypto typically process in 16,384 byte (16k) chunks and pads them? I'm not familiar with ChaCha20 or the encryption requirements of HAP.

What is the best way to test if the crypto is to blame?

I think the issue is somewhere in Cryptographer.swift, specifically, in the encrypt function:

func encrypt(_ data: Data) throws -> Data {
        defer { encryptCount += 1 }
        logger.debug("Encrypt message #\(self.encryptCount)")

        let nonce = encryptCount.bigEndian.bytes
        let length = UInt16(data.count).bigEndian.bytes
        logger.debug("Message: \(data.hex), Nonce: \(nonce.hex), Length: \(length.hex)")

        let encrypted = try ChaCha20Poly1305.encrypt(message: data,
                                                     additional: length,
                                                     nonce: nonce,
                                                     key: encryptKey)
        logger.debug("Cipher: \((length + encrypted).hex)")

        return length + encrypted
 }
Bouke commented 6 years ago

Some other implementations encrypt the message in chunks of 0x400 (1024) bytes:

Reading up on the specs, chapter 5.5.2 specifies:

Each HTTP message is split into frames no larger than 1024 bytes.

See 2447710decc24a0aef1f5f85c7fc61ef23b9d476 which resolves this issue. I've verified the fix with 99 bridged accessories, resulting in a message length of 69.765 bytes.

Bouke commented 6 years ago

I'm not sure why this issue manifested at messages exceeding 16k, as messages exceeding 1k are already in violation of the specs.

Messages from the controller should also adhere to this spec, however the old implementation had a precondition (trap) that would be hit if messages were split. I haven't come across a situation where the controller sends such large messages. Anyway that should be fixed now too, although I haven't found a way to test this from an actual iOS device.

Thanks for the input!

dmavromatis commented 6 years ago

Confirmed -- it works here as well.