Review Request: PROTON-C: add pedantic frame header checks and max length validation

[Kenneth Giusti <kg...@apache.org>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12011/
-----------------------------------------------------------

Review request for qpid and Rafael Schloming.


Description
-------

Added max frame length checking, and general frame header validation.


This addresses bug proton-109.
    https://issues.apache.org/jira/browse/proton-109


Diffs
-----

  /proton/trunk/proton-c/include/proton/error.h 1494614 
  /proton/trunk/proton-c/include/proton/framing.h 1494614 
  /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1494614 
  /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1494614 
  /proton/trunk/proton-c/src/engine/engine-internal.h 1494614 
  /proton/trunk/proton-c/src/engine/engine.c 1494614 
  /proton/trunk/proton-c/src/framing/framing.c 1494614 
  /proton/trunk/proton-c/src/proton-dump.c 1494614 
  /proton/trunk/proton-c/src/sasl/sasl.c 1494614 
  /proton/trunk/tests/python/proton_tests/engine.py 1494614 

Diff: https://reviews.apache.org/r/12011/diff/


Testing
-------

added new python unit tests for max frame violation, and header field validation.


Thanks,

Kenneth Giusti

Re: Review Request 12011: PROTON-C: add pedantic frame header checks and max length validation

[Rafael Schloming <rh...@apache.org>]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12011/#review24868
-----------------------------------------------------------



/proton/trunk/proton-c/src/dispatcher/dispatcher.c
<https://reviews.apache.org/r/12011/#comment49035>

    Given that dispatcher is just a utility class used by transport and sasl, it doesn't need to allocate its own error object. You could just pass in a reference to the error object from the container (either transport or sasl). This way you wouldn't need to propogate the error text back up, it would just automatically get set to the correct thing.



/proton/trunk/proton-c/src/dispatcher/dispatcher.c
<https://reviews.apache.org/r/12011/#comment49038>

    I don't know how much this matters, but you are going to end up parsing the frame size twice with this check done this way. 
    
    Given that pn_read_frame is very lightweight, you could just do the check after calling pn_read_frame. If pn_read_frame succeeds then you have a syntactically well formed frame that just might be semantically larger than permitted (which is ok because we haven't committed any resources to processing it yet) and you can error after looking at frame.size. On the other hand, if pn_read_frame returns 0 then you can check if available is > the max frame size and error in that case.



/proton/trunk/proton-c/src/engine/engine-internal.h
<https://reviews.apache.org/r/12011/#comment49036>

    Why did you have to add this here? It wouldn't make sense to use from outside the transport, would it?



/proton/trunk/proton-c/src/framing/framing.c
<https://reviews.apache.org/r/12011/#comment49034>

    I'd suggest we change the signature to an ssize_t and return the consumed amount | error. With the current signature it looks like it might consume some amount even when there is an error.



/proton/trunk/proton-c/src/sasl/sasl.c
<https://reviews.apache.org/r/12011/#comment49037>

    Hmm, interesting. Ok, I understand why you put pn_do_error in engine-internal.h, however won't this result in a close frame getting sent prior to any open frame being sent? I believe there is advice in the sasl spec (or some similar security spec) to simply abort the connection entirely with no information when encountering an error during this stage of setup. I can see why that would be unfriendly for debugging, however it seems sensible for production systems. If we do want to support something more friendly for debugging we should actually make sure it's a properly formed frame sequence, i.e. an open followed immediately by a close rather than just a close frame.



/proton/trunk/tests/python/proton_tests/engine.py
<https://reviews.apache.org/r/12011/#comment49039>

    Note that this particular test assumes an eager approach to detecting the max frame size error and wouldn't work with what I described above. In fact this is assuming that you are going to incrementally parse each sub field of the header as it comes in rather than waiting until you have at least 8 bytes before bothering to try to parse the thing.
    
    In the past I've tried both approaches (push parsing and waiting until you have enough bytes and then doing a straightforward procedural decode) and the latter has been more successful for me both performance-wise and in terms of code simplicity. So I'd say the tests shouldn't assume that partial headers will necessarily generate any result.


- Rafael Schloming


On June 20, 2013, 5:30 p.m., Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/12011/
> -----------------------------------------------------------
> 
> (Updated June 20, 2013, 5:30 p.m.)
> 
> 
> Review request for qpid and Rafael Schloming.
> 
> 
> Bugs: proton-109
>     https://issues.apache.org/jira/browse/proton-109
> 
> 
> Repository: qpid
> 
> 
> Description
> -------
> 
> Added max frame length checking, and general frame header validation.
> 
> 
> Diffs
> -----
> 
>   /proton/trunk/proton-c/include/proton/error.h 1494614 
>   /proton/trunk/proton-c/include/proton/framing.h 1494614 
>   /proton/trunk/proton-c/src/dispatcher/dispatcher.h 1494614 
>   /proton/trunk/proton-c/src/dispatcher/dispatcher.c 1494614 
>   /proton/trunk/proton-c/src/engine/engine-internal.h 1494614 
>   /proton/trunk/proton-c/src/engine/engine.c 1494614 
>   /proton/trunk/proton-c/src/framing/framing.c 1494614 
>   /proton/trunk/proton-c/src/proton-dump.c 1494614 
>   /proton/trunk/proton-c/src/sasl/sasl.c 1494614 
>   /proton/trunk/tests/python/proton_tests/engine.py 1494614 
> 
> Diff: https://reviews.apache.org/r/12011/diff/
> 
> 
> Testing
> -------
> 
> added new python unit tests for max frame violation, and header field validation.
> 
> 
> Thanks,
> 
> Kenneth Giusti
> 
>