temp-file creation vulnerability in rdbSave function

[megahall]

I have been trying to reach the Redis maintainers since 2013-09-13
regarding this report, but I could not find a good security contact for
Redis, and the lead maintainer, Salvatore Sanfilippo <antirez@gmail.com>
is not replying to my private report to him about the issue and his
opinion of it. I also contacted US-CERT for help and they could not
reach anyone by 2014-01-24.

Therefore I would like to encourage the Redis team to be more
security-friendly and establish some contact procedures on their
website. Given how many places this software is now being used these
days, I think it is very critical to make these changes before someone
finds something more serious than the one I could spot.

I think I might have discovered a security vulnerability in Redis
2.6.16. This code is from the function int rdbSave(char *filename) in rdb.c:

   630  int rdbSave(char *filename) {
   631      dictIterator *di =3D NULL;
   632      dictEntry *de;
   633      char tmpfile[256];
   634      char magic[10];
   635      int j;
   636      long long now =3D mstime();
   637      FILE *fp;
   638      rio rdb;
   639      uint64_t cksum;
   640
   641      snprintf(tmpfile,256,"temp-%d.rdb", (int) getpid());
   642      fp =3D fopen(tmpfile,"w");
   643      if (!fp) {
   644          redisLog(REDIS_WARNING, "Failed opening .rdb for saving: %s",
   645              strerror(errno));
   646          return REDIS_ERR;
   647      }
   ...
   692      /* Make sure data will not remain on the OS's output buffers */
   693      fflush(fp);
   694      fsync(fileno(fp));
   695      fclose(fp);
   696
   697      /* Use RENAME to make sure the DB file is changed atomically only
   698       * if the generate DB file is ok. */
   699      if (rename(tmpfile,filename) =3D=3D -1) {
   700          redisLog(REDIS_WARNING,"Error moving temp DB file on the final destination: %s", strerror(errno));
   701          unlink(tmpfile);
   702          return REDIS_ERR;
   703      }

In line 641, the function does not use a security temporary file creation
routine such as mkstemp. This is vulnerable to a wide range of attacks which
could result in overwriting (in line 693-695) and unlinking (in line 701) any
file / hard link / symlink placed in temp-PID.rdb by an attacker.

https://www.owasp.org/index.php/Improper_temp_file_opening

https://www.owasp.org/index.php/Insecure_Temporary_File

The code should be creating the temporary file using some kind of safe
function like mkstemp, O_EXCL open, etc. instead of just using a PID value
which does not have enough entropy and protection from race conditions. It
should also be sure it has set the CWD of itself to a known-safe location that
should have permissions which are only open to the redis daemon / redis user
and not to other users or processes.

The advisory is posted here:

https://www.mhcomputing.net/redis-advisory-2013.txt

Thanks,
Matthew Hall

[fgeek]

@megahall does this issue have CVE identifier? Please do request one publicly from oss-security. Documentation: http://people.redhat.com/kseifrie/CVE-OpenSource-Request-HOWTO.html

[megahall]

Requested one. I'll post it when I get it.

[cicku]

CC

[antirez]

Hello, the reason I did not replied is because this security bug report is absurd.

Security must be evaluated in the context of a given software. With Redis CONFIG SET you can make the server chdir to whatever directory you want, fill the DB with a given set of keys that will result in a specific RDB file, change the name of the target file, and use SAVE in order to write random files with mostly attacker-chosen content around the filesystem.

In the light of the above, do you think that the symlink vulnerability in Redis is significant?

[megahall]

1. At least somebody could reply and say it's absurd, even if they don't say anything else. And at least somebody could have a security contact process on your website for people to report any other vulnerabilities which might be less "absurd". ;)
2. The "CONFIG SET" can be renamed to something else secret to plug the hole in your example. But no mitigation is present for the symlink issue though it's more minor.
3. If a distribution packages Redis running with any sort of elevated privileges or FS permissions a lot of damage could be done, in terms of lateral movement by attackers.
4. This bug might be small but I think it points to other issues people should be thinking about when using the product. Maybe there could be some recommendations for hardening the installation to make it as safe as one can given all the types of things we're discussing?

Matthew.

Sent from my mobile device.

On April 23, 2014 7:02:32 AM PDT, Salvatore Sanfilippo notifications@github.com wrote:

Hello, the reason I did not replied is because this security bug report
is absurd.

Security must be evaluated in the context of a given software. With
Redis CONFIG SET you can make the server chdir to whatever directory
you want, fill the DB with a given set of keys that will result in a
specific RDB file, change the name of the target file, and use SAVE in
order to write random files with mostly attacker-chosen content around
the filesystem.

In the light of the above, do you think that the symlink vulnerability
in Redis is significant?

---

Reply to this email directly or view it on GitHub:
#1560 (comment)

[yossigo]

While this issue involves temporary file creation, it does not involve typical security issues with temporary files.

We assume dir is not writable (or possibly even readable) by attackers. If it is, that's a configuration error that would result with many other issues, involving practically every file Redis reads or writes.

Because of that, the usual issues around choosing a non-existing random temp file name, handling the inherent race condition, opening exclusively etc. don't apply.

Closing this issue now, and I will verify the documentation does indeed mention the need for a secure dir.