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

2.1.26 plugins/scram.c decode_saslname() returns corrupted authz name #416

Closed
wbclay opened this issue Jan 19, 2017 · 1 comment
Closed

Comments

@wbclay
Copy link

wbclay commented Jan 19, 2017

decode_saslname() decodes (un-escapes) its input string in place, but does not \0-terminate the resulting decoded string.

If decoding reduces the input string's length, trailing original (escaped) characters are appended at the end of the returned string. An incorrectly failed authorization may ensue because the returned value may fail match criteria that a correctly-decoded SASL name would pass.

I have identified one case in which this incorrect authorization failure occurs consistently: a SASL SCRAM-SHA-1 auth with a "dn:" style authzID. The 1-line patch below at line 154 correctly yields successful authorization. There may be other cases in which this bug can produce incorrect authorization failures, but I have not identified them.

An unrelated suspected logic error:

While seeking the above bug, I noticed another possible coding error.

2.1.26 scram_server_mech_step1() line 623 reads:

_plug_strdup(sparams->utils, text->authorization_id, &text->authorization_id, NULL);

It appears to me that _plug_strdup() will not operate properly if its "in" and "&out" arguments resolve to the same pointer address. In particular, "strcpy((char *) *out, in);" will not copy the input string because the preceding utils->malloc() call will have replaced the "in" pointer value with a newly-allocated *out pointer value.

The three-line patch below at lines 498 and 623 would eliminate this possible error.

NB: I have not identified a case to demonstrate this logic is wrong. I apologize for this false alarm if the analysis above is incorrect.

Suggested patches:

bill@fuji:/usr/local/src/cyrus-sasl-2.1.26/plugins$ diff -e scram.c.orig scram.c
623c
authorization_id = text->authorization_id; // _plug_strdup in and *out must not be the same address!
_plug_strdup(sparams->utils, authorization_id, &text->authorization_id, NULL);
.
498a
char * authorization_id;
.
154a
*outp = '\0';
.
bill@fuji:/usr/local/src/cyrus-sasl-2.1.26/plugins$

ksmurchison added a commit that referenced this issue Jul 13, 2017
… authz name (using modified patch from wbclay)
@ksmurchison
Copy link
Contributor

Fixed with 14fd13b

brong pushed a commit that referenced this issue Jan 29, 2018
… authz name (using modified patch from wbclay)
brong pushed a commit that referenced this issue Jan 30, 2018
… authz name (using modified patch from wbclay)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants