buglwIP - A Lightweight TCP/IP stack - Bugs: bug #52836, altcp_write error because of...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #52836: altcp_write error because of invalid sndbuf available returned

Submitter:  David GIRAULT <dgirault>
Submitted:  Mon 08 Jan 2018 02:12:42 PM UTC
   
 
Category:  apps Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  git head

Jump to the original submission

Thu 25 Jan 2018 12:16:09 PM UTC, comment #8: 

Should be fixed again.

Simon Goldschmidt <goldsimon>
Group administrator
Wed 24 Jan 2018 02:59:52 PM UTC, comment #7: 

Well, if compression is enabled, we should expect enough gain to leave space for `ssl_added`, right? So I think it should be ok to return inner sndbuf.

David GIRAULT <dgirault>
Mon 22 Jan 2018 06:57:45 PM UTC, comment #6: 

Nice idea. Just what do we do if this function fails (e.g. if compression is enabled)? Is it OK to return the inner sndbuf in this case?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 22 Jan 2018 03:54:29 PM UTC, comment #5: 

Hi Simon,

FYI, just found the following function in mbedTLS that will return the exact value required for the ssl_added variable.

int mbedtls_ssl_get_record_expansion( const mbedtls_ssl_context *ssl )

It take care of the negociated cipher suite, and record header size.

So I think it may be used instead of the obvious calculation I use in may orignial patch.

David GIRAULT <dgirault>
Mon 15 Jan 2018 03:46:32 PM UTC, comment #4: 

Sorry, have no time to reply before.

No problem for your editing, I try to push bugs I found in an early state so it can be discussed. Patch I gave are subject to change (and this one wasn't rebase to latest master of lwip du to lack of time)...

For your questions, the ssl_added variable include
- the SSL record header which is 5 bytes,
- the 8 bytes IV use to encrypt the record,
- the 16 bytes TAG use to authenticate the record,
- 3 additional bytes to keep a size aligned (may not be necessary),
- 16 additional bytes for security because the IV and TAG size may change and if TAG is 32 bytes (SHA512 for example), you loose because calculated sndbuf will be to big.

Since I don't known how I can gather the size of IV and TAG negotiated by SSL during handshake, I put some conservative value here.

If you know some mbedTLS functions that can do this job, it will be great to use them.

Sorry I don't understand you question in comment #2, but what I see in `mbedtls_ssl_write` is that it always fail if provided buffer size is > negociated fragment/record size and previous fragment is always flushed before, so it always start with a new fragment. It result `sndbuf` MUST be <= `mbedtls_ssl_get_max_frag_len()`.


David GIRAULT <dgirault>
Thu 11 Jan 2018 10:19:47 AM UTC, comment #3: 

Pushed slightly changed. I hope you don't mind that you're the committer and I edited it...

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Jan 2018 08:52:10 PM UTC, comment #2: 

Also, does the "sndbuf" depend on the fragment size? I would have thought a larger write would come through as long as the additional overhead of a 2nd (3rd...) fragment fits the sndbuf. Although it's probably not worth the calculation?

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Jan 2018 08:47:32 PM UTC, comment #1: 

And again, I can't apply this patch to my sources :-(

The idea is good, but:
- size_t ssl_added = 5+8+16+3+16;": WTF? These should be constants! Ideally pointing to defines from mbedtls, but at least make them literals so anyone has a chance to see where these values come from (and if they are correct or stay correct in the future)
- "The last '+3+16' is for alignment & security.": alignment also has controlling defines. I assume +3 means 4-byte-alignment. What does 16 stand for?

Don't get me wrong: we do need this to make sendbuf() work as expected. It's just not ready yet.

Simon Goldschmidt <goldsimon>
Group administrator
Mon 08 Jan 2018 02:12:42 PM UTC, original submission:  

While using MQTT over an SSL connexion, using MQTT_OUTPUT_RINGBUF_SIZE == 8192, we got error when altcp_write() when our MQTT message reach 2*MSS.

mbedTLS version of altcp API use altcp_default_sndbuf() which return tcp_sendbuf(), but when writing using this max value on a altcp_mbedtls socket, this return an error because the generated ciphered version of the message is greater than the tcp_sndbuf() of lower TCP socket.

This patch add a new altcp_mbedtls_sndbuf() implementation that take care of record header and more data added by TLS to data to be sent.

David GIRAULT <dgirault>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attached Files

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by dziegel (Updated the item)
  • -email is unavailable- added by goldsimon (Posted a comment)
  • -email is unavailable- added by dgirault (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 7 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-01-25 goldsimon Open/ClosedOpen Closed
    2018-01-24 dziegel Open/ClosedClosed Open
    2018-01-11 goldsimon StatusIn Progress Fixed
        Open/ClosedOpen Closed
    2018-01-08 goldsimon StatusNone In Progress
        Assigned toNone goldsimon
    2018-01-08 dgirault Attached File- Added altcp_mbedtls_sndbuf.patch, #42870

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code