<story><title>Systemd is bad parsing</title><url>https://blog.erratasec.com/2018/10/systemd-is-bad-parsing-and-should-feel.html</url></story><parent_chain><item><author>ambrop7</author><text>Yes the code is bad but does not need to be completely rewritten. It should interpret the data as an array uint8_t (no casting to structure&amp;#x2F;integer pointers!), use simple helper functions to read out (u)int(8&amp;#x2F;16&amp;#x2F;32) values (there was a post on HN about this recently, &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;commandcenter.blogspot.com&amp;#x2F;2012&amp;#x2F;04&amp;#x2F;byte-order-fallacy.html&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;commandcenter.blogspot.com&amp;#x2F;2012&amp;#x2F;04&amp;#x2F;byte-order-fallac...&lt;/a&gt;), and be careful to check things like sizes.&lt;p&gt;The code is also wrong because of strict aliasing. This is a real problem, your program can in fact exhibit undefined behavior because of this (it happened to me).&lt;p&gt;Some time ago I wrote some code to make these things simpler in C++. It allows you to define &amp;quot;structures&amp;quot; (which however are not real structs, it&amp;#x27;s an abstraction) and then directly access an arbitrary buffer through setters&amp;#x2F;getters. The code is here with documentation: &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstack&amp;#x2F;infra&amp;#x2F;Struct.h&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstac...&lt;/a&gt; . In fact this is part of my own TCP&amp;#x2F;IP stack and it includes DHCP code  (here are my DHCP structure definitions: &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstack&amp;#x2F;proto&amp;#x2F;DhcpProto.h&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstac...&lt;/a&gt;).</text></item></parent_chain><comment><author>moefh</author><text>Interestingly (and pedantically), the code from the otherwise excellent &amp;quot;Byte order fallacy&amp;quot; article technically contains undefined behavior in C on machines where int is 32 bits wide (which is almost everything nowadays).&lt;p&gt;Consider this example of extracting a 32-bit int encoded as little endian:&lt;p&gt;&lt;pre&gt;&lt;code&gt;    #include &amp;lt;stdio.h&amp;gt;

    int main(void)
    {
      unsigned char data[4] = { 0, 0, 0, 128 };
      unsigned int i;

      &amp;#x2F;&amp;#x2F; the line of code from the article:
      i = (data[0]&amp;lt;&amp;lt;0) | (data[1]&amp;lt;&amp;lt;8) | (data[2]&amp;lt;&amp;lt;16) | (data[3]&amp;lt;&amp;lt;24);
    
      printf(&amp;quot;%x\n&amp;quot;, i);
    }
&lt;/code&gt;&lt;/pre&gt;
Compiling this with either gcc or clang using &amp;quot;-fsanitize=undefined&amp;quot; triggers this runtime error:&lt;p&gt;&lt;pre&gt;&lt;code&gt;    test.c:9:61: runtime error: left shift of 128 by 24 places cannot be represented in type &amp;#x27;int&amp;#x27;
&lt;/code&gt;&lt;/pre&gt;
That happens because, even though the bytes are unsigned as the article requires (using uint8_t instead of unsigned char has the same problem), they are promoted to (signed) int before the left shift as part of automatic integer promotion. When the left shift by 24 happens, it puts an 1 into the sign bit of the int, which is undefined behavior. Later the int is converted to unsigned (using implementation-defined behavior, which luckily is sane on all popular compilers), but by then it&amp;#x27;s already too late.&lt;p&gt;The solution is to change the code slightly:&lt;p&gt;&lt;pre&gt;&lt;code&gt;   i = (data[0]&amp;lt;&amp;lt;0) | (data[1]&amp;lt;&amp;lt;8) | (data[2]&amp;lt;&amp;lt;16) | ((unsigned)data[3]&amp;lt;&amp;lt;24);
&lt;/code&gt;&lt;/pre&gt;
This prevents the promotion from unsigned char (or uint8_t) to int by explicitly casting it to an unsigned int, which can then be safely shifted left by 24.</text></comment>
<story><title>Systemd is bad parsing</title><url>https://blog.erratasec.com/2018/10/systemd-is-bad-parsing-and-should-feel.html</url></story><parent_chain><item><author>ambrop7</author><text>Yes the code is bad but does not need to be completely rewritten. It should interpret the data as an array uint8_t (no casting to structure&amp;#x2F;integer pointers!), use simple helper functions to read out (u)int(8&amp;#x2F;16&amp;#x2F;32) values (there was a post on HN about this recently, &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;commandcenter.blogspot.com&amp;#x2F;2012&amp;#x2F;04&amp;#x2F;byte-order-fallacy.html&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;commandcenter.blogspot.com&amp;#x2F;2012&amp;#x2F;04&amp;#x2F;byte-order-fallac...&lt;/a&gt;), and be careful to check things like sizes.&lt;p&gt;The code is also wrong because of strict aliasing. This is a real problem, your program can in fact exhibit undefined behavior because of this (it happened to me).&lt;p&gt;Some time ago I wrote some code to make these things simpler in C++. It allows you to define &amp;quot;structures&amp;quot; (which however are not real structs, it&amp;#x27;s an abstraction) and then directly access an arbitrary buffer through setters&amp;#x2F;getters. The code is here with documentation: &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstack&amp;#x2F;infra&amp;#x2F;Struct.h&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstac...&lt;/a&gt; . In fact this is part of my own TCP&amp;#x2F;IP stack and it includes DHCP code  (here are my DHCP structure definitions: &lt;a href=&quot;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstack&amp;#x2F;proto&amp;#x2F;DhcpProto.h&quot; rel=&quot;nofollow&quot;&gt;https:&amp;#x2F;&amp;#x2F;github.com&amp;#x2F;ambrop72&amp;#x2F;aipstack&amp;#x2F;blob&amp;#x2F;master&amp;#x2F;src&amp;#x2F;aipstac...&lt;/a&gt;).</text></item></parent_chain><comment><author>camgunz</author><text>Yeah casting to struct pointers is a bright red flag.  It also rarely saves you anything since packed structs are slow, and you still have to validate the contents.&lt;p&gt;You&amp;#x27;re right, these streams should be interpreted as uint8_t to avoid undefined behavior when shifting, masking, and ORing, and anyone doing this should tuck it behind some abstraction layer.  It should never bleed into parsing.</text></comment>