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

Submitted by:  David GIRAULT <dgirault>
Submitted on:  Mon 08 Jan 2018 02:12:42 PM UTC  
 
Category:  apps Severity:  3 - Normal
Item Group:  None Status:  Fixed
Privacy:  Public Assigned to:  Simon Goldschmidt <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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>
Project AdministratorIn charge of this item.
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>

 

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)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    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.5