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

racecondition() #736

Closed
RootUp opened this issue Jul 15, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@RootUp
Copy link

commented Jul 15, 2018

Team,

File: /htslib/blob/develop/cram/cram_io.c

        snprintf(fai_file, PATH_MAX, "%s.fai", fn);
        if (access(fai_file, R_OK) != 0)
            if (fai_build(fn) != 0)
                return NULL;
}

I believe this indicates a security flaw, If an attacker can change anything along the path between the call access() and the files actually used, attacker may exploit the race condition.

Request team to have a look and validate.

@jkbonfield

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

This logic appears in several places sadly, and I think I just duplicated another bit of code when I wrote that (without spotting the race - sorry!). Eg https://github.com/samtools/samtools/blob/develop/sam.c#L129-L146

We probably need a new fai_build which refuses to run when the file already exists (via exclusive opens) so that all acces && build idioms are fixed together.

jkbonfield added a commit to jkbonfield/htslib that referenced this issue Jul 16, 2018

Fixes a race condition in building fai indices.
The API for the fai_build* functions is to take a filename.  This
inherently leaves a race.  Code currently does things like
    if (!access(...)) fai_build()
or
    if (!open(...)) fai_build().

Both of these suffer the same problem - an attacker could symlink the
file between the access/open check and the decision to rebuild it.

This solution makes the build function itself unlink the file and does
an exclusive open so it'll fail if someone wins the race between
unlink and hopen.

There is one small fly in the ointment - we can currently do "samtools
faidx s3:foo.fa" and this will hopen s3:foo.fa.fai.  The unlink will
fail, so we've now changed behaviour because previously we could
overwrite an existing s3 fai file with a new one whereas now we
require the user to manually delete their existing one first.  Without
having an hunlink() function we're scuppered on this.  The only
possible workaround is to validate the filename first, so if the fasta
file is fd_backend we use exclusive open, and don't otherwise.  (This
is what is implemented here.)

Fixes samtools#736
@jmarshall

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

This is based on a report from FlawFinder, which has essentially just grepped for /access/.

Considering that the file path is usually user-supplied, I think it would be worth thinking through the real security concerns here and their gravity, before leaping to avoid access().

@jkbonfield

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

The problem isn't access specifically. It happens with open too. Eg see the code path used by samtools faidx.

I thought about it and there is a small but limited issue. If you're in a directory with write access (eg /tmp) and the fai file doesn't yet exist, then you can in theory have a race condition to get the caller of samtools to overwrite another file that they have access to by careful timing on making a symlink. It's even possible to make this a non-race on things like samtools faidx by simply creating the symlink and waiting.

The risk is minuscule, but the fix is also trivial too and doesn't remove the access call. Unless you can think of a situation where the fix breaks things, I'm inclined to go with it.

@jmarshall

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

The difference between opening a file for write, and unlinking a file then opening that filename for write-create, is that you create a fresh inode.

So if you have multiple hard links to .fa and .fa.fai, the hard link for .fa.fai will be broken (so the other file locations won't be updated). If you have specially set up permissions or ownership, those will be lost on the .fa.fai file. If your foo.fa and foo.fa.fai are symlinks to the files in some repository directory, the link will be broken and the repository directory's .fa.fai won't be updated.

I'm not doing any of those things myself for .fai files that I'm likely to (intentionally) regenerate, but it doesn't seem that outlandish. The proposed fix changes behaviour in such cases.

If you're working in a shared-write directory, can't you can be induced to write to the wrong file just in the shell? e.g. samtools view foo.bam > vulnerable.sam

@daviesrob

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

Hmm, yes. I think we'll have to park this one for now until we've had a bit more time to think about it.

@RootUp

This comment has been minimized.

Copy link
Author

commented Jul 17, 2018

Not sure, but this can be exploited like this,

exploit.c

#include <unistd.h>
int main()
{
  if (geteuid()!=0) exit(1);
  setuid(geteuid());
  char *args[] = { "/bin/sh", 0 };
  return execve(args[0], args, 0);
}

Trigger an race condition by creating a hardlink to the vulnerable program.
while :; do ln -f ./vulnpoc; ln -f ./exploit; done

@jkbonfield

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

No "this" cannot be exploited like this. Your example has no bearing at all on what samtools is doing.

We're not doing any execution, chowning, and nor would we ever recommend people running their bioinformatics analysis as root! All we're doing here is opening a file and writing some tab delimited text index to it. The attacker can't even control the contents of this file as that is entirely based on the contents of the user supplied fasta file.

The problem comes down to having access to a writeable location where the fasta but not fasta.fai file is present. It's an esoteric corner case, and @jmarshall fairly points out it's not too dissimilar to any shell redirection of "foo > bar" with bar being raced as a symlink to somewhere else.

I hadn't really thought about the hardlink case, although again affecting those only applies to writeable / shared locations, so it's also high improbable to cause bugs either way. However I agree it's probably not worth riskiing other bugs in order to fix this extremely esoteric case.

@jkbonfield

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2018

We discussed this in our weekly meeting and concluded fixing this is likely to cause just as many problems as it solves, and that the actual issue is so slight it's more of a user education problem than a realistic security flaw. Ie won't fix.

@jkbonfield jkbonfield closed this Jul 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.