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

Off by one in _sasl_add_string function #587

Closed
stze opened this issue Nov 28, 2019 · 6 comments · Fixed by #663
Closed

Off by one in _sasl_add_string function #587

stze opened this issue Nov 28, 2019 · 6 comments · Fixed by #663

Comments

@stze
Copy link

stze commented Nov 28, 2019

Dear Cyrus SASL team —

During tests against openldap 2.4.48, I have detected an off-by-one error in _sasl_add_string function. In case of openldap this bug can cause a denial-of-service condition or has other unspecified impact.

Valgrind output from openldap

5ddfddde do_bind: dn () SASL mech <garbage>
5ddfddde ==> sasl_bind: dn="" mech=<garbage>
datalen=0
==11019== Thread 3:
==11019== Invalid write of size 1
==11019==    at 0x4B9B1DB: sasl_seterror (seterror.c:247)
==11019==    by 0x4B9A18D: sasl_server_start (server.c:1418)
==11019==    by 0x26B88B: slap_sasl_bind (sasl.c:1666)
==11019==    by 0x21E130: fe_op_bind (bind.c:279)
==11019==    by 0x21DCE1: do_bind (bind.c:205)
==11019==    by 0x1F35BA: connection_operation (connection.c:1185)
==11019==    by 0x1F3CE7: connection_read_thread (connection.c:1342)
==11019==    by 0x35DFF9: ldap_int_thread_pool_wrapper (tpool.c:1048)
==11019==    by 0x4DBE668: start_thread (pthread_create.c:479)
==11019==    by 0x4EFA322: clone (clone.S:95)
==11019==  Address 0x62032a8 is 0 bytes after a block of size 600 alloc'd
==11019==    at 0x483CFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==11019==    by 0x4B930A4: _buf_alloc (common.c:2186)
==11019==    by 0x4B93299: _sasl_add_string (common.c:196)
==11019==    by 0x4B9B2D4: sasl_seterror (seterror.c:187)
==11019==    by 0x4B9A18D: sasl_server_start (server.c:1418)
==11019==    by 0x26B88B: slap_sasl_bind (sasl.c:1666)
==11019==    by 0x21E130: fe_op_bind (bind.c:279)
==11019==    by 0x21DCE1: do_bind (bind.c:205)
==11019==    by 0x1F35BA: connection_operation (connection.c:1185)
==11019==    by 0x1F3CE7: connection_read_thread (connection.c:1342)
==11019==    by 0x35DFF9: ldap_int_thread_pool_wrapper (tpool.c:1048)
==11019==    by 0x4DBE668: start_thread (pthread_create.c:479)
==11019==
==11019== Invalid read of size 1
==11019==    at 0x483DF54: strlen (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==11019==    by 0x4E53DE4: __vfprintf_internal (vfprintf-internal.c:1688)
==11019==    by 0x4E67029: __vsnprintf_internal (vsnprintf.c:114)
==11019==    by 0x3A1FFA: lutil_debug (debug.c:74)
==11019==    by 0x266FF3: slap_sasl_log (sasl.c:146)
==11019==    by 0x4B9B4CF: sasl_seterror (seterror.c:260)
==11019==    by 0x4B9A18D: sasl_server_start (server.c:1418)
==11019==    by 0x26B88B: slap_sasl_bind (sasl.c:1666)
==11019==    by 0x21E130: fe_op_bind (bind.c:279)
==11019==    by 0x21DCE1: do_bind (bind.c:205)
==11019==    by 0x1F35BA: connection_operation (connection.c:1185)
==11019==    by 0x1F3CE7: connection_read_thread (connection.c:1342)
==11019==  Address 0x62032a8 is 0 bytes after a block of size 600 alloc'd
==11019==    at 0x483CFAF: realloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==11019==    by 0x4B930A4: _buf_alloc (common.c:2186)
==11019==    by 0x4B93299: _sasl_add_string (common.c:196)
==11019==    by 0x4B9B2D4: sasl_seterror (seterror.c:187)
==11019==    by 0x4B9A18D: sasl_server_start (server.c:1418)
==11019==    by 0x26B88B: slap_sasl_bind (sasl.c:1666)
==11019==    by 0x21E130: fe_op_bind (bind.c:279)
==11019==    by 0x21DCE1: do_bind (bind.c:205)
==11019==    by 0x1F35BA: connection_operation (connection.c:1185)
==11019==    by 0x1F3CE7: connection_read_thread (connection.c:1342)
==11019==    by 0x35DFF9: ldap_int_thread_pool_wrapper (tpool.c:1048)
==11019==    by 0x4DBE668: start_thread (pthread_create.c:479)

Patch

diff --git a/lib/common.c b/lib/common.c
index bc3bf1df..9969d6aa 100644
--- a/lib/common.c
+++ b/lib/common.c
@@ -190,7 +190,7 @@ int _sasl_add_string(char **out, size_t *alloclen,

   if (add==NULL) add = "(null)";

-  addlen=strlen(add); /* only compute once */
+  addlen=strlen(add)+1; /* only compute once */
   if (_buf_alloc(out, alloclen, (*outlen)+addlen)!=SASL_OK)
     return SASL_NOMEM;

Please let me know what additional information I can provide to fix the issue.

-Stephan Zeisberg

@hyc
Copy link
Contributor

hyc commented Nov 28, 2019

Fyi, was originally reported to us and diagnosed here http://www.openldap.org/its/index.cgi/Incoming?id=9123

@Neustradamus
Copy link
Contributor

If we can have a new release with a solution for this problem...
Thanks to @quanah who gives me the information about this ticket :)

@carnil
Copy link

carnil commented Dec 19, 2019

CVE-2019-19906 was assigned for this issue.

@quanah quanah closed this as completed in dcc9f51 Feb 18, 2020
quanah added a commit that referenced this issue Feb 18, 2020
Off by one error in common.c, CVE-2019-19906.

Thanks to Stephan Zeisberg for reporting
iboukris pushed a commit to iboukris/cyrus-sasl that referenced this issue Feb 24, 2020
Off by one error in common.c, CVE-2019-19906.

Thanks to Stephan Zeisberg for reporting
aiobofh pushed a commit to aiobofh/cyrus-sasl that referenced this issue Mar 4, 2020
Off by one error in common.c, CVE-2019-19906.

Thanks to Stephan Zeisberg for reporting
GuidoKiener added a commit to GuidoKiener/cyrus-sasl that referenced this issue Feb 7, 2021
Issue cyrusimap#587 was not solved correct.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.
GuidoKiener added a commit to GuidoKiener/cyrus-sasl that referenced this issue Jun 26, 2021
Issue cyrusimap#587 was not solved correct.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Guido Kiener <guido@kiener-muenchen.de>
quanah pushed a commit that referenced this issue Jun 26, 2021
Issue #587 was not solved correct.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Guido Kiener <guido@kiener-muenchen.de>
@bmanahilov
Copy link

CVE-2019-19906 was assigned for this issue.

I see the @GuidoKiener 's fix that was merged is on code that already existed in 2.1.25.
Do you know why this CVE-2019-19906 is referencing 2.1.27 only.
Didn't the same issue already exist in 2.1.25?
Thanks.

hyc added a commit to hyc/cyrus-sasl that referenced this issue Sep 22, 2021
Issue cyrusimap#587 was not solved correctly.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Howard Chu <hyc@symas.com>
hyc added a commit to hyc/cyrus-sasl that referenced this issue Sep 22, 2021
Issue cyrusimap#587 was not solved correctly.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Howard Chu <hyc@symas.com>
hyc added a commit to hyc/cyrus-sasl that referenced this issue Sep 22, 2021
Just a cleaned up version of the fix.

Signed-off-by: Howard Chu <hyc@symas.com>
quanah pushed a commit that referenced this issue Sep 23, 2021
Just a cleaned up version of the fix.

Signed-off-by: Howard Chu <hyc@symas.com>
quanah pushed a commit that referenced this issue Sep 23, 2021
Issue #587 was not solved correct.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Guido Kiener <guido@kiener-muenchen.de>
quanah pushed a commit that referenced this issue Sep 23, 2021
Just a cleaned up version of the fix.

Signed-off-by: Howard Chu <hyc@symas.com>
quanah pushed a commit that referenced this issue Sep 29, 2021
Issue #587 was not solved correct.

_sasl_add_string adds zero terminator to the output string.
This cuts log messages after the first '%s' of the format string.
With the fix the function _sasl_log now logs the complete message.

Signed-off-by: Guido Kiener <guido@kiener-muenchen.de>
quanah pushed a commit that referenced this issue Sep 29, 2021
Just a cleaned up version of the fix.

Signed-off-by: Howard Chu <hyc@symas.com>
@flyn-org
Copy link

Will we see a release soon with this fix? CVE-2019-19906 has a base score of high. We are in the process of addressing this in OpenWrt with a local patch:

openwrt/packages#17114
openwrt/packages#17113
openwrt/packages#17112

@quanah
Copy link
Contributor

quanah commented Nov 12, 2021

There's one remaining security issue open that needs addressing before a new release.

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

Successfully merging a pull request may close this issue.

7 participants