Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The channel binding data dynbuf was not set correctly #15685

Closed
galen11 opened this issue Dec 4, 2024 · 3 comments
Closed

The channel binding data dynbuf was not set correctly #15685

galen11 opened this issue Dec 4, 2024 · 3 comments
Assignees

Comments

@galen11
Copy link

galen11 commented Dec 4, 2024

I did this

‎lib/http_negotiate.c

/* Check if the connection is using SSL and get the channel binding data */
#ifdef HAVE_GSSAPI
  if(conn->handler->flags & PROTOPT_SSL) {
    Curl_dyn_init(&neg_ctx->channel_binding_data, SSL_CB_MAX_SIZE);
    result = Curl_ssl_get_channel_binding(
      data, FIRSTSOCKET, &neg_ctx->channel_binding_data);
    if(result) {
      Curl_http_auth_cleanup_negotiate(conn);
      return result;
    }
  }
#endif

The channel binding data dynbuf was not set correctly. toobig should be set to the actual data length plus 1. Otherwise, dyn_nappend will fail with CURLE_TOO_LARGE.
To address this, toobig should be set to SSL_CB_MAX_SIZE + 1 when Curl_dyn_init.

The issue was from an enhancement of cURL, see 0a5ea09

I expected the following

No response

curl/libcurl version

curl 8.10.1

operating system

RedHat / Rocky Linux

@bagder
Copy link
Member

bagder commented Dec 5, 2024

Thanks for this report. A question on this though:

The dynbuf logic looks like this:

  if(fit > s->toobig) {
    return CURLE_TOO_LARGE;

... which I think indicates that the limit set in dyn_init() is the maximum allowed size. And the comments say 85 bytes is the largest allowed size.

So where is the discrepancy?

@bagder bagder self-assigned this Dec 5, 2024
@galen11
Copy link
Author

galen11 commented Dec 5, 2024

struct dynbuf {
  char *bufr;    /* point to a null-terminated allocated buffer */
  size_t leng;   /* number of bytes *EXCLUDING* the null-terminator */
  size_t allc;   /* size of the current allocation */
  size_t toobig; /* size limit for the buffer */
...
};
/*
 * Store/append an chunk of memory to the dynbuf.
 */
static CURLcode dyn_nappend(struct dynbuf *s,
                            const unsigned char *mem, size_t len)
{
  size_t indx = s->leng;
  size_t a = s->allc;
  size_t fit = len + indx + 1; /* new string + old string + zero byte */

...

  if(fit > s->toobig) {
    Curl_dyn_free(s);
    return CURLE_TOO_LARGE;
  }

"leng" is the actual data length (EXCLUDING the null-terminator).
"fit" is the actual data length plus 1 byte null-terminator.
"toobig" should also be the actual data length plus 1 byte null-terminator, so that we can do "if(fit > s->toobig)" as expected.
For example, if the actual data length is just 1 byte, what should we set for "toobig"? We should set it to 2.
Back to this case, the largest allowed size is 85 bytes but which is the actual data length, "fit" will be 86, so we should set "toobig" to "86", which is SSL_CB_MAX_SIZE + 1.

@bagder
Copy link
Member

bagder commented Dec 5, 2024

Yes, you're right of course. That's also why the value is called "toobig", So max_allowed + 1 is a suitable toobig argument. I'll make a PR for this now.

bagder added a commit that referenced this issue Dec 5, 2024
The channel binding data dynbuf was not set correctly making it fail with
CURLE_TOO_LARGE too easily.

Reported-by: galen11 on github
Fixes #15685
@bagder bagder closed this as completed in 6755ba5 Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants