Compressed dns #3769

jonatanolofsson · 2017-10-28T23:53:44Z

According to (RFC 1035), clients must understand compressed dns messages. These commits aim to add the support of this to the ESP8266mDNS library.

devyte · 2017-12-12T03:20:59Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp

@@ -539,15 +539,24 @@ void MDNSResponder::_parsePacket(){
    while (numAnswers--) {
      // Read name
      stringsRead = 0;
+      size_t here = 0;


Please rename this into something more meaningful

devyte · 2017-12-12T03:31:46Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp

@@ -632,31 +649,36 @@ void MDNSResponder::_parsePacket(){
        uint16_t answerPrio = _conn_read16(); // Read priority
        uint16_t answerWeight = _conn_read16(); // Read weight
        answerPort = _conn_read16(); // Read port
+        size_t here = 0;


Doesn't this declaration of here shadow the declaration on line 542?

jonatanolofsson · 2017-12-12T08:28:52Z

Thank you for the feedback, I updated the code accordingly.

devyte

I don't know the changes of compressed dns, but the code changes look ok.

devyte · 2018-01-03T03:21:43Z

Help needed with testing this!

jonatanolofsson · 2018-01-07T16:22:20Z

The way I discovered this was missing was that I got errors with mDNS when connecting to a mac (which is compressing its data). Thus, to test this, set up a local domain with dns-sd with and without this patch and monitor the esp8266 errors on the serial port as it tries to connect (compile with DEBUG_ESP_MDNS_RX)

devyte · 2018-01-07T16:46:38Z

@jonatanolofsson my wife has a mac. I hate the thing, but I could use it to test this, if you provide detailed instructions for the setup on it.

d-a-v · 2018-01-07T20:53:05Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp

        if (tmp8 == 0x00) { // End of name
          break;
        }
+        if (tmp8 & 0xC0) { // Compressed pointer
+          uint16_t offset = ((((uint16_t)tmp8) & 0xC0) << 8) | _conn_read8();


Would the resulting offset's boundaries be checked here ?

If the first byte is 0xca and the second 0xaa, then the offset would be 0xcaaa.
Would it not be 0xaaa like with (((uint16_t)(tmp8 & ~0xC0)) << 8) | _conn_read8() ?

d-a-v · 2018-01-08T13:31:36Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp


        (void) answerPrio;
        (void) answerWeight;

        // Read hostname
        tmp8 = _conn_read8();
-        if (tmp8 & 0xC0) { // Compressed pointer (not supported)
+        if (tmp8 & 0xC0) { // Compressed pointer
+          uint16_t offset = ((((uint16_t)tmp8) & 0xC0) << 8) | _conn_read8();


Is it &~0xc0 too here ?

d-a-v · 2018-01-08T13:34:10Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp


        (void) answerPrio;
        (void) answerWeight;

        // Read hostname
        tmp8 = _conn_read8();
-        if (tmp8 & 0xC0) { // Compressed pointer (not supported)
+        if (tmp8 & 0xC0) { // Compressed pointer
+          uint16_t offset = ((((uint16_t)tmp8) & 0xC0) << 8) | _conn_read8();


Do you think the calculated offset can be checked beeing out of bound to prevent crashing with malformed or crafted dns replies ?

d-a-v · 2018-01-08T16:13:06Z

libraries/ESP8266WiFi/src/include/UdpContext.h

+    }
+
+    bool isValidOffset(const size_t pos) const {
+        return (pos <= _rx_buf->len);


Thanks.
Since you are going to use this offset, would'nt it be pos < _rx_buf->len ?
Sorry I don't mean to be pushy :]

Not at all, I appreciate your comments, I just havn't had much time to do more than fix them during breaks at work today =D
The check on line 222 in the 'read' function checks if the offset is at least equal to the limit, which should give that to set the buffer as "fully read", the offset should be set just past the limit, i.e. equal to the length. I figured "finished reading" was a valid use-case and thus a valid offset.

Thanks for the explanation

d-a-v · 2018-01-09T20:49:54Z

libraries/ESP8266mDNS/ESP8266mDNS.cpp

+              DEBUG_ESP_PORT.println(offset);
+#endif
+              _conn->seek(offset);
+              tmp8 = _conn_read8();


Are you reading tmp8 twice (here and in L568) ?

Indeed, I forgot to remove the second one. Fixed.

d-a-v · 2018-01-10T12:12:21Z

@jonatanolofsson can you tell us more about the steps to test your PR ?
Wife also has a mac. Wives...

jonatanolofsson · 2018-01-10T13:25:38Z

I'll try to find time to make a test description. Not going to happen this week unfortunately. My computer is a mac, but I'm only ever booting into linux anymore :)

d-a-v · 2018-01-10T13:32:57Z

Linux is no problem if you know how to update avahi's conffile so.

jonatanolofsson · 2018-01-10T13:35:04Z

Right, I wasn't sure if avahi compressed the payload

devyte · 2018-03-16T03:32:31Z

@jonatanolofsson I now understand your code changes, and I think they are correct. I am currently rewriting the mdns code based on the idf functionality, and was struggling a bit with compression (it's very different in idf due to fundamental differences in how things work at low level). This helps quite a bit. I'll merge this, and integrate it into my current work.

* Add UdpContext seek and tell * Add support for DNS compressed messages * mDNS compressed pointer: Validate offset before jumping (cherry picked from commit 461c922)

jonatanolofsson force-pushed the develop/compressed-dns branch from 7ea02cf to c10892a Compare December 1, 2017 11:13

jonatanolofsson force-pushed the develop/compressed-dns branch from c10892a to 3f03ef0 Compare December 11, 2017 12:44

jonatanolofsson mentioned this pull request Dec 11, 2017

Compressed DNS #3946

Closed

devyte requested changes Dec 12, 2017

View reviewed changes

jonatanolofsson force-pushed the develop/compressed-dns branch 2 times, most recently from e3a3a00 to 873a173 Compare December 12, 2017 08:25

jonatanolofsson force-pushed the develop/compressed-dns branch from 873a173 to bd6c859 Compare December 16, 2017 08:21

jonatanolofsson force-pushed the develop/compressed-dns branch from bd6c859 to 7065571 Compare January 1, 2018 17:22

devyte approved these changes Jan 3, 2018

View reviewed changes

devyte added the help wanted Help needed from the community label Jan 5, 2018

d-a-v reviewed Jan 7, 2018

View reviewed changes

jonatanolofsson force-pushed the develop/compressed-dns branch from d8d1467 to 7f645e0 Compare January 8, 2018 10:57

d-a-v reviewed Jan 8, 2018

View reviewed changes

jonatanolofsson force-pushed the develop/compressed-dns branch 2 times, most recently from dcd569f to 1adb42f Compare January 8, 2018 15:02

d-a-v reviewed Jan 8, 2018

View reviewed changes

d-a-v approved these changes Jan 8, 2018

View reviewed changes

d-a-v reviewed Jan 9, 2018

View reviewed changes

jonatanolofsson force-pushed the develop/compressed-dns branch from 1adb42f to f81af25 Compare January 10, 2018 13:01

jonatanolofsson force-pushed the develop/compressed-dns branch from f81af25 to 2570bdc Compare February 12, 2018 17:43

jonatanolofsson added 2 commits March 6, 2018 22:51

Add UdpContext seek and tell

5b0a1df

Add support for DNS compressed messages

85bbb38

mDNS compressed pointer: Validate offset before jumping

1d702d5

jonatanolofsson force-pushed the develop/compressed-dns branch from 2570bdc to 1d702d5 Compare March 6, 2018 21:52

Merge branch 'master' into develop/compressed-dns

2e315c0

devyte merged commit 461c922 into esp8266:master Mar 16, 2018

d-a-v mentioned this pull request Apr 18, 2018

with OOM debug option: section type conflict with __c (PSTR) #4650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compressed dns #3769

Compressed dns #3769

jonatanolofsson commented Oct 28, 2017

devyte Dec 12, 2017

devyte Dec 12, 2017

jonatanolofsson commented Dec 12, 2017

devyte left a comment

devyte commented Jan 3, 2018

jonatanolofsson commented Jan 7, 2018

devyte commented Jan 7, 2018

d-a-v Jan 7, 2018 •

edited

Loading

d-a-v Jan 8, 2018

d-a-v Jan 8, 2018

d-a-v Jan 8, 2018

jonatanolofsson Jan 8, 2018

d-a-v Jan 8, 2018

d-a-v Jan 9, 2018

jonatanolofsson Jan 10, 2018

d-a-v commented Jan 10, 2018

jonatanolofsson commented Jan 10, 2018

d-a-v commented Jan 10, 2018

jonatanolofsson commented Jan 10, 2018

devyte commented Mar 16, 2018

Compressed dns #3769

Compressed dns #3769

Conversation

jonatanolofsson commented Oct 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonatanolofsson commented Dec 12, 2017

devyte left a comment

Choose a reason for hiding this comment

devyte commented Jan 3, 2018

jonatanolofsson commented Jan 7, 2018

devyte commented Jan 7, 2018

d-a-v Jan 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-a-v commented Jan 10, 2018

jonatanolofsson commented Jan 10, 2018

d-a-v commented Jan 10, 2018

jonatanolofsson commented Jan 10, 2018

devyte commented Mar 16, 2018

d-a-v Jan 7, 2018 •

edited

Loading