Missing null termination in PROTOCOL_BINARY_CMD_SASL_LIST_MECHS response handling

Reporting here as it might be security relevant and the upstream is dead

Thank you fr your report, sadly it was missed to be picked up so far - I'm subscribing the security Team for their thoughts on the issue.

Status changed to 'Confirmed' because the bug affects multiple users.

This looks like a duplicate of #1381160.

The attachment "lp1573594_disco.debdiff" seems to be a debdiff. The ubuntu-sponsors team has been subscribed to the bug report so that they can review and hopefully sponsor the debdiff. If the attachment isn't a patch, please remove the "patch" flag from the attachment, remove the "patch" tag, and if you are member of the ~ubuntu-sponsors, unsubscribe the team.

[This is an automated message performed by a Launchpad user owned by ~brian-murray, for any issue please contact him.]

Hi @joalif,

couple changes needed in the debdiffs, please:

1) the patch "debian/patches/fix_missing_null_termination" has no suffix...
   not a major problem but convention is to suffix it with ".diff" or ".patch"
2) The line in your patch:
+ * Fix missing null terminated buffer. Closes: #853497.
   should be removed; the text is redundant with the line above it, and the
   patch doesn't actually fix Debian bug 853497 (as far as I can tell, at
   least). Also, Ubuntu debdiffs shouldn't include Debian-specific Closes:
   tags...which leads to:
3) Your changelog entry contains "(Closes: #1573594)", but Ubuntu uses
   "(LP: #NNNNNN)" tag formatting. Please change your changelog entry to
   include "(LP: #1573594)" instead.
4) All your debdiffs include the same package version: 1.0.18-4.2ubuntu1
   This is a tricky point of pkg versioning; since they are currently
   identical in multiple releases, it's best to use a release-specific
   version number for each of the SRU releases, and only use the "ubuntu1"
   suffix for the development (i.e. disco) release.
   Specifically to clarify:
     release current version new version
     Xenial 1.0.18-4.1 1.0.18-4.1ubuntu1
     Bionic 1.0.18-4.2 1.0.18-4.2ubuntu0.18.04.1
     Cosmic 1.0.18-4.2 1.0.18-4.2ubuntu0.18.10.1
     Disco 1.0.18-4.2 1.0.18-4.2ubuntu1

I can make the changes to the debdiffs you already uploaded if you want, or please upload new debdiffs with the recommended changes.

Thanks!

New debdiff for disco.

New debdiff for bionic.

New debdiff for bionic.

New debdiff for xenial.

@joalif,

I'll gladly sponsor this SRU for you ... but I'm a little bit concerned about the test case field being empty and I'm sure the SRU team (ppl who approved upload) will do as well.

While I can understand there is no clear reproducer for that bug ...
Could you please prove somehow that you did some dogfooding ?
Use libmemcached in a context as close as possible to the modified code ?
Ran a libmemcached/memcached test suite (if any) ?

Anything that can bring confidence in this patch. Especially that we have no upstream maintainer to review/accept it.

Regards,
Eric

Thanks Ionna for the [test case] improvement.
Sponsored for Disco, once the package is found in disco-release, we can go ahead with the SRU for T,X,B,C.

- Eric

This bug was fixed in the package libmemcached - 1.0.18-4.2ubuntu1

---------------
libmemcached (1.0.18-4.2ubuntu1) disco; urgency=medium

  * d/p/fixing_missing_null_termination.patch:
    - Fix missing null termination in PROTOCOL_BINARY_CMD_SASL_LIST_MECHS
      response handling (LP: #1573594)

 -- Ioanna Alifieraki <email address hidden> Fri, 18 Jan 2019 13:04:25 +0000

Note:
---
* No longer active upstream
  - https://code.launchpad.net/libmemcached
  - Last Modified: 2014-02-16

* Patch has been submitted Debian
  - https://bugs.debian.org/919696
---

Under the circumstances of libmemcached being no longer active upstream and considering the fact that @Joalif filed a bug and submitted the patch to Debian:

Sponsored for C/B/X/T.

@Joalif, when the releases will turn "Fix Committed". The package will start building and soon be found in $release-proposed for the testing phase.

Please test and describe (with details) your tests and results against each libmemcached proposed packages. It will serves as a justification for the SRU team to approve the final copy from -proposed to -updates (final destination marking the end of the SRU) if no regression found.

Regards,
Eric

[Additionnal Sponsor note]

---
Xenial
---
Rejected:
File libmemcached_1.0.18-4.1ubuntu1.debian.tar.xz already exists in Primary Archive for Ubuntu, but uploaded version has different contents. See more information about this error in https://help.launchpad.net/Packaging/UploadErrors.
Files specified in DSC are broken or missing, skipping package unpack verification.
---

I had to bump the version for Xenial from "1.0.18-4ubuntu1" to "1.0.18-4ubuntu2".
"1.0.18-4ubuntu1" has already been uploaded/built back in 2015 and got superseded/deleted for some reasons that I not aware of, therefore can't be use again.

# https://launchpad.net/ubuntu/+source/libmemcached/+publishinghistory

Date Status Target Pocket Component Section Version
2015-12-12 11:54:14 EST Superseded Xenial release main libs 1.0.18-4ubuntu1
2015-12-13 13:10:09 EST Deleted Xenial proposed main libs 1.0.18-4ubuntu1

# Approved upload:
[ubuntu/xenial-proposed] libmemcached 1.0.18-4.1ubuntu2 (Waiting for approval)

Hello Stefan, or anyone else affected,

Accepted libmemcached into cosmic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libmemcached/1.0.18-4.2ubuntu0.18.10.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-cosmic to verification-done-cosmic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-cosmic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Stefan, or anyone else affected,

Accepted libmemcached into bionic-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libmemcached/1.0.18-4.2ubuntu0.18.04.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-bionic to verification-done-bionic. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-bionic. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Stefan, or anyone else affected,

Accepted libmemcached into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libmemcached/1.0.18-4.1ubuntu2 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

Hello Stefan, or anyone else affected,

Accepted libmemcached into trusty-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/libmemcached/1.0.8-1ubuntu2.1 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed. Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-trusty to verification-done-trusty. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-trusty. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance for helping!

N.B. The updated package will be released to -updates after the bug(s) fixed by this package have been verified and the package has been in -proposed for a minimum of 7 days.

I accepted this to -proposed, but since this seems to be a security-related fix, I would really want the security team to take a look at the change and decide whether this should actually go through -security or both.

Thanks Łukasz, this looks appropriate for an SRU update.

Thanks

Verification on Xenial :

Repeating the steps described in the [Test Case] of bug description :

Install the xenial-proposed packages

$ dpkg -l | grep libmem
ii libhashkit-dev 1.0.18-4.1ubuntu2 amd64 libmemcached hashing functions and algorithms (development files)
ii libhashkit2:amd64 1.0.18-4.1ubuntu2 amd64 libmemcached hashing functions and algorithms
ii libmemcached-dev 1.0.18-4.1ubuntu2 amd64 C and C++ client library to the memcached server (development files)
ii libmemcached-tools 1.0.18-4.1ubuntu2 amd64 Commandline tools for talking to memcached via libmemcached
ii libmemcached11:amd64 1.0.18-4.1ubuntu2 amd64 C and C++ client library to the memcached server
ii libmemcachedutil2:amd64 1.0.18-4.1ubuntu2 amd64 library implementing connection pooling for libmemcached

$ ./sasl_test 88BAB0 1A99094B77C8935ED9F1461C767DB1F9 mc2.dev.eu.ec2.memcachier.com
Get/Set success!

Verification on Bionic :

Repeating the steps described in the [Test Case] of bug description :

Install the bionic-proposed packages

$ dpkg -l | grep libmemcache
ii libhashkit-dev:amd64 1.0.18-4.2 amd64 libmemcached hashing functions and algorithms (development files)
ii libhashkit2:amd64 1.0.18-4.2 amd64 libmemcached hashing functions and algorithms
ii libmemcached-dev:amd64 1.0.18-4.2 amd64 C and C++ client library to the memcached server (development files)
ii libmemcached-tools 1.0.18-4.2 amd64 Commandline tools for talking to memcached via libmemcached
ii libmemcached11:amd64 1.0.18-4.2 amd64 C and C++ client library to the memcached server
ii libmemcachedutil2:amd64 1.0.18-4.2 amd64 library implementing connection pooling for libmemcached

$ ./sasl_test 88BAB0 1A99094B77C8935ED9F1461C767DB1F9 mc2.dev.eu.ec2.memcachier.com
Get/Set success!

Verification on Cosmic :

Repeating the steps described in the [Test Case] of bug description :

Install the cosmic-proposed packages

$ dpkg -l | grep libmemcached
ii libhashkit-dev:amd64 1.0.18-4.2ubuntu0.18.10.1 amd64 libmemcached hashing functions and algorithms (development files)
ii libhashkit2:amd64 1.0.18-4.2ubuntu0.18.10.1 amd64 libmemcached hashing functions and algorithms
ii libmemcached-dev:amd64 1.0.18-4.2ubuntu0.18.10.1 amd64 C and C++ client library to the memcached server (development files)
ii libmemcached-tools 1.0.18-4.2ubuntu0.18.10.1 amd64 Commandline tools for talking to memcached via libmemcached
ii libmemcached11:amd64 1.0.18-4.2ubuntu0.18.10.1 amd64 C and C++ client library to the memcached server
ii libmemcachedutil2:amd64 1.0.18-4.2ubuntu0.18.10.1 amd64 library implementing connection pooling for libmemcached

$ ./sasl_test 88BAB0 1A99094B77C8935ED9F1461C767DB1F9 mc2.dev.eu.ec2.memcachier.com
Get/Set success!

Correction on comment #28 :

Verification on Bionic
Repeating the steps described in the [Test Case] of bug description :

Install the bionic-proposed packages

$ dpkg -l | grep libmem
ii libhashkit-dev:amd64 1.0.18-4.2ubuntu0.18.04.1 amd64 libmemcached hashing functions and algorithms (development files)
ii libhashkit2:amd64 1.0.18-4.2ubuntu0.18.04.1 amd64 libmemcached hashing functions and algorithms
ii libmemcached-dev:amd64 1.0.18-4.2ubuntu0.18.04.1 amd64 C and C++ client library to the memcached server (development files)
ii libmemcached-tools 1.0.18-4.2ubuntu0.18.04.1 amd64 Commandline tools for talking to memcached via libmemcached
ii libmemcached11:amd64 1.0.18-4.2ubuntu0.18.04.1 amd64 C and C++ client library to the memcached server
ii libmemcachedutil2:amd64 1.0.18-4.2ubuntu0.18.04.1 amd64 library implementing connection pooling for libmemcached

$ ./sasl_test 88BAB0 1A99094B77C8935ED9F1461C767DB1F9 mc2.dev.eu.ec2.memcachier.com
Get/Set success!

Verification on Trusty :

**Verification failed**
libmemcached on Trusty does not support SASL authentication

Repeating the steps described in the [Test Case] of bug description :

Install the trusty-proposed packages

$ dpkg -l | grep libmemcached
ii libhashkit-dev 1.0.8-1ubuntu2.1 amd64 libmemcached hashing functions and algorithms (development files)
ii libhashkit2:amd64 1.0.8-1ubuntu2.1 amd64 libmemcached hashing functions and algorithms
ii libmemcached-dev 1.0.8-1ubuntu2.1 amd64 C and C++ client library to the memcached server (development files)
ii libmemcached-tools 1.0.8-1ubuntu2.1 amd64 Commandline tools for talking to memcached via libmemcached
ii libmemcached10:amd64 1.0.8-1ubuntu2.1 amd64 C and C++ client library to the memcached server
ii libmemcachedprotocol0:amd64 1.0.8-1ubuntu2.1 amd64 library implementing the memcached protocol
ii libmemcachedutil2:amd64 1.0.8-1ubuntu2.1 amd64 library implementing connection pooling for libmemcached

$ ./sasl_test 88BAB0 1A99094B77C8935ED9F1461C767DB1F9 mc2.dev.eu.ec2.memcachier.com
Couldn't setup SASL auth: ACTION NOT SUPPORTED
Set failed: AUTHENTICATION FAILURE

Ionna,

Let's then request the SRU verification team to drop the package for trusty-proposed.
If SASL is not supported in the Trusty pkg, there is no point to complete the SRU for Trusty.

Additionally, since Trusty is near to its EOL, I don't see good reason/justification to justify the effort/work to enable SASL on the package.

Also enabling SASL, IMHO, won't be consider a "bug fix", but a "new feature".

For all the reasons above, let's simply drop the pkg from trusty-proposed.

- Eric

Thanks for verifying! I'll drop the trusty package from -proposed and release the others into -updates.

This bug was fixed in the package libmemcached - 1.0.18-4.2ubuntu0.18.10.1

---------------
libmemcached (1.0.18-4.2ubuntu0.18.10.1) cosmic; urgency=medium

  * d/p/fixing_missing_null_termination.patch:
    - Fix missing null termination in PROTOCOL_BINARY_CMD_SASL_LIST_MECHS
      response handling (LP: #1573594)

 -- Ioanna Alifieraki <email address hidden> Fri, 18 Jan 2019 13:36:33 +0000

The verification of the Stable Release Update for libmemcached has completed successfully and the package has now been released to -updates. Subsequently, the Ubuntu Stable Release Updates Team is being unsubscribed and will not receive messages about this bug report. In the event that you encounter a regression using the package from -updates please report a new bug using ubuntu-bug and tag the bug report regression-update so we can easily find any regressions.

This bug was fixed in the package libmemcached - 1.0.18-4.2ubuntu0.18.04.1

---------------
libmemcached (1.0.18-4.2ubuntu0.18.04.1) bionic; urgency=medium

  * d/p/fixing_missing_null_termination.patch:
    - Fix missing null termination in PROTOCOL_BINARY_CMD_SASL_LIST_MECHS
      response handling (LP: #1573594)

 -- Ioanna Alifieraki <email address hidden> Fri, 18 Jan 2019 12:41:28 +0000

This bug was fixed in the package libmemcached - 1.0.18-4.1ubuntu2

---------------
libmemcached (1.0.18-4.1ubuntu2) xenial; urgency=medium

  * d/p/fixing_missing_null_termination.patch:
    - Fix missing null termination in PROTOCOL_BINARY_CMD_SASL_LIST_MECHS
      response handling (LP: #1573594)

 -- Ioanna Alifieraki <email address hidden> Fri, 18 Jan 2019 12:57:57 +0000

Hi,

From the code:
memcached_return_t rc= memcached_response(server, mech, sizeof(mech), NULL);
  if (memcached_failed(rc))

memcached_response() adds NULL to strings if memcached_response() successful, otherwise the code follows the error path. The author of this report mentions:
libmemcached/response.cc:619
which is:
          if ((rc= memcached_safe_read(instance, buffer, bodylen)) != MEMCACHED_SUCCESS)
          {
            return MEMCACHED_UNKNOWN_READ_FAILURE;
          }

As you can see, an error is returned back to memcached_sasl as mentioned in the original report.

All that you will achieve with calling memset() in this manner is hide any real bug that valgrind would find.

The reason why "This bug is difficult to reproduce since it depends on the contents of the stack.", is because there is no bug in the reporters work.

I cannot find a case of this bug being reported upstream, otherwise I would close it there with above message.

Thanks,

   -- Brian

FWIW I appreciate people going to the effort of reporting bugs, it takes time to write them up.

> memcached_response() adds NULL to strings
Please point to the place where null is appended.

> As you can see, an error is returned back to memcached_sasl as mentioned in the original report.

Where does it say in the original report that an error is returned?

> there is no bug in the reporters work
This bug was observed in an actual production environment with information getting leaked, not based merely on code analysis.

Please provide a test case clearly showing the bug in the upstream project.

Calling memset() would be incorrect in this case even if you were concerned about a NULL character ( i.e. the minimal change would be to insert a NULL character in the buffer after looked at the length of the returned string from memcached_response()... but you don't have to do that because it is already done for you ).

I mentioned the error path only because in that path there is no guarantee that a NULL would be added to the buffer, but then the buffer passed to memcached_response() goes unused.

> Calling memset() would be incorrect in this case even if you were concerned about a NULL character
How is it incorrect? memcached_response never writes the last byte of the buffer, so whatever it writes will be guaranteed to be null terminated.

> i.e. the minimal change would be to insert a NULL character in the buffer after looked at the length of the returned string from memcached_response()
You can't look at the length of the returned string as the buffer is already corrupted at that point and memcached_response does not return the length either. That's why the buffer needs to be zeroed before passing it in. There's a reason multiple people reviewed and signed off on this patch.

> but you don't have to do that because it is already done for you
> I mentioned the error path only because in that path there is no guarantee that a NULL would be added to the buffer

Once again, you fail to cite where this is ostensibly "already done". The non-error path does not add a null termination, I double checked.

"You can't look at the length of the returned string as the buffer is already corrupted at that point and memcached_response does not return the length either."

For you to understand why what your saying is off, you will need to spend sometime understanding how memcached_response(), which internal API call, is used in the code.

FWIW I spoke to Dormando, who has the maintainer Memcached for anyone's recent memory. The binary protocol is/being deprecated, which in turns means that SASL support will be dropped ( it was only ever supported for the binary protocol ).