Re: [dev] Miscellaneous sbase issues
+1 for option 3) Why would anybody want to trust somebody that creates malicious archives like that? A symlink in an archive should just be a symlink, nothing more. -Truls
Re: [dev] Miscellaneous sbase issues
On Mon, Apr 27, 2015 at 08:12:42PM +0100, Nick wrote: > One thing the patch doesn't cover is an archive using a symlink to > somewhere like ../../ and then putting a file in symlink/newfile > (hence sending it to ../../newfile). I only thought of that when > reading the bsdtar manpage[0]. > > I'm not sure what the best behaviour is in that case. Some options: > > 1) If a symlink creates a path ascending further up the directory > tree (towards /) than the current directory, replace all symlinks in > the path with directories and extract there. > > 2) Remove the symlink and replace it with a real directory before > extracting the file into it (this is the behaviour of bsdtar with > the -U option) > > 3) Refuse to create any file following a symlink (this is the > default behaviour of bsdtar) > > 4) Issue a warning if writing a file, but follow the symlink as > instructed > > If you're curious about how bsdtar does it, look at check_symlinks() > in libarchive/archive_write_disk_posix.c, but note the comment at > the top of the function - "TODO: Make this work." > > I have a slight preference for option 1, but it doesn't feel > particularly clean. Anyone else have better ideas? I know it's > annoying, but I don't think "ignore it" is good enough, as it would > be far too easy to create a tarball that blatted any file the user > had access to using this method (and using -t to check paths > wouldn't help, as the ../ is in the symlink target). I'm attaching a > tarball that demonstrates the problem, in case I haven't explained > it well enough. If you used $HOME/bin/, and unpacked the tarball in > $HOME/tmp, it will create a file in $HOME/bin/myscript. > > Nick > > 0. > https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/bsdtar.1.html I am not sure what the proper approach is. Option 3) sounds pretty safe as a starting point. Any ideas?
Re: [dev] Miscellaneous sbase issues
Quoth Nick: > Quoth Dimitris Papastamos: > > Some things that need to be done for tar: > > > > ... > > - Strip leading / from filenames and dangerous things like ../../ etc. > > OK, attached is a patch that does that. I think it covers all the > bases. One thing the patch doesn't cover is an archive using a symlink to somewhere like ../../ and then putting a file in symlink/newfile (hence sending it to ../../newfile). I only thought of that when reading the bsdtar manpage[0]. I'm not sure what the best behaviour is in that case. Some options: 1) If a symlink creates a path ascending further up the directory tree (towards /) than the current directory, replace all symlinks in the path with directories and extract there. 2) Remove the symlink and replace it with a real directory before extracting the file into it (this is the behaviour of bsdtar with the -U option) 3) Refuse to create any file following a symlink (this is the default behaviour of bsdtar) 4) Issue a warning if writing a file, but follow the symlink as instructed If you're curious about how bsdtar does it, look at check_symlinks() in libarchive/archive_write_disk_posix.c, but note the comment at the top of the function - "TODO: Make this work." I have a slight preference for option 1, but it doesn't feel particularly clean. Anyone else have better ideas? I know it's annoying, but I don't think "ignore it" is good enough, as it would be far too easy to create a tarball that blatted any file the user had access to using this method (and using -t to check paths wouldn't help, as the ../ is in the symlink target). I'm attaching a tarball that demonstrates the problem, in case I haven't explained it well enough. If you used $HOME/bin/, and unpacked the tarball in $HOME/tmp, it will create a file in $HOME/bin/myscript. Nick 0. https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man1/bsdtar.1.html test.tar Description: Unix tar archive signature.asc Description: Digital signature
Re: [dev] Miscellaneous sbase issues
On Sun, 26 Apr 2015 18:24:18 -0700 Michael Forney wrote: Hi Michael, > printf > -- > Ignores flag characters '#', '0', '-', ' ', and '+', but is labeled as > POSIX compliant and complete, so this is presumably unintentional. > > "git am" breaks without this functionality. I fixed this in the latest commit, thanks for reporting! Stay tuned for it to be pushed to the suckless repositories, it's still in staging. Please let me know if "git am" works now. :) Cheers FRIGN -- FRIGN
Re: [dev] Miscellaneous sbase issues
Quoth Dimitris Papastamos:
> Some things that need to be done for tar:
>
> ...
> - Strip leading / from filenames and dangerous things like ../../ etc.
OK, attached is a patch that does that. I think it covers all the
bases.
>From b5acf1e9254080c2f283c623f59e412cdb29939a Mon Sep 17 00:00:00 2001
From: Nick White
Date: Mon, 27 Apr 2015 12:25:44 +0100
Subject: [PATCH] Strip dangerous path traversal stuff from paths
Specifically, this removes '../' from anywhere in a path, and any
number of '/' at the start of a path
---
tar.c | 22 ++
1 file changed, 22 insertions(+)
diff --git a/tar.c b/tar.c
index b1f3b27..0bd3fcf 100644
--- a/tar.c
+++ b/tar.c
@@ -359,6 +359,27 @@ sanitize(struct header *h)
}
static void
+sanitizepath(char *p)
+{
+ size_t l;
+ char *s;
+
+ /* Strip leading '/' characters */
+ while(*p == '/') {
+ l = strlen(p);
+ memmove(p, p+1, l - 1);
+ *(p + l - 1) = '\0';
+ }
+
+ /* Strip '../' from anywhere */
+ while((s = strstr(p, "../")) != NULL) {
+ l = strlen(s);
+ memmove(s, s + 3, l - 3);
+ *(s + l - 3) = '\0';
+ }
+}
+
+static void
chktar(struct header *h)
{
char tmp[8], *err;
@@ -407,6 +428,7 @@ xt(int argc, char *argv[], int (*fn)(char *, ssize_t, char[BLKSIZ]))
(int)sizeof(h->prefix), h->prefix);
snprintf(fname + n, sizeof(fname) - n, "%.*s",
(int)sizeof(h->name), h->name);
+ sanitizepath(fname);
if ((size = strtol(h->size, &p, 8)) < 0 || *p != '\0')
eprintf("strtol %s: invalid number\n", h->size);
--
1.7.10.4
Re: [dev] Miscellaneous sbase issues
On Sun, Apr 26, 2015 at 06:24:18PM -0700, Michael Forney wrote: > tar > --- > Since fb1595a69c091a6f6a9303b1fab19360b876d114, tar calls remove(3) on > directories before extracting them. I'm not sure that it is reasonable > for tar to do this because users may want to re-extract archives, or > extract archives on top a directory structure that already exists. > Additionally, it is fairly common to find tar archives containing the > "." directory (possibly with a trailing '/'), which were constructed > using "tar -cf foo.tar .". Yeah that makes sense I suppose. Some things that need to be done for tar: - Investigate aforementioned remove vs unlink issue. - When we tar a file, we need to ensure to use both name/prefix if the filename is more than 100 chars. - Strip leading / from filenames and dangerous things like ../../ etc. > cat, tee > > These utilities read from stdin using fread(3) into a buffer of size > BUFSIZ. However, fread will read until it fills up the entire buffer (or > hits EOF) before returning, causing noticeable delay when the input > comes from other programs or scripts. > > To demonstrate this problem, compare the output of these commands: > > for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done > for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | cat > for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | tee > > I considered fixing this by making the concat function take an fd > instead and make a single call to read(2), but this causes problems for > sponge, which uses a FILE * obtained from tmpfile(3) as both output and > input for concat. We could also use mkstemp(3) to return a file > descriptor, and use a FILE * from fdopen for writing, and the file > descriptor for reading, but this seems unclean to me. We should avoid mixing file stream I/O and raw I/O. Check out '2.5.1 Interaction of File Descriptors and Standard I/O Streams'. > Another option would be to use fgetc and fputc for the concat > implementation, and let libc take care of the buffering. I'm not sure if > this has any performance implications. Sounds about right.
Re: [dev] Miscellaneous sbase issues
On April 27, 2015 3:24:18 AM CEST, Michael Forney wrote: >Hi suckless, > >I came across some issues in sbase whose solution wasn't immediately >apparent: > >printf >-- >Ignores flag characters '#', '0', '-', ' ', and '+', but is labeled as >POSIX compliant and complete, so this is presumably unintentional. > >"git am" breaks without this functionality. > >tar >--- >Since fb1595a69c091a6f6a9303b1fab19360b876d114, tar calls remove(3) on >directories before extracting them. I'm not sure that it is reasonable >for tar to do this because users may want to re-extract archives, or >extract archives on top a directory structure that already exists. >Additionally, it is fairly common to find tar archives containing the >"." directory (possibly with a trailing '/'), which were constructed >using "tar -cf foo.tar .". > >cat, tee > >These utilities read from stdin using fread(3) into a buffer of size >BUFSIZ. However, fread will read until it fills up the entire buffer >(or >hits EOF) before returning, causing noticeable delay when the input >comes from other programs or scripts. > >To demonstrate this problem, compare the output of these commands: > > for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done >for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | >cat >for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | >tee > >I considered fixing this by making the concat function take an fd >instead and make a single call to read(2), but this causes problems for >sponge, which uses a FILE * obtained from tmpfile(3) as both output and >input for concat. We could also use mkstemp(3) to return a file >descriptor, and use a FILE * from fdopen for writing, and the file >descriptor for reading, but this seems unclean to me. > >Another option would be to use fgetc and fputc for the concat >implementation, and let libc take care of the buffering. I'm not sure >if >this has any performance implications. > > >Thanks everyone for their work on sbase! It really has come a long way >in the last 6 months. Firstly, patch..? For cat and tee fgets might be more appropriate.
[dev] Miscellaneous sbase issues
Hi suckless, I came across some issues in sbase whose solution wasn't immediately apparent: printf -- Ignores flag characters '#', '0', '-', ' ', and '+', but is labeled as POSIX compliant and complete, so this is presumably unintentional. "git am" breaks without this functionality. tar --- Since fb1595a69c091a6f6a9303b1fab19360b876d114, tar calls remove(3) on directories before extracting them. I'm not sure that it is reasonable for tar to do this because users may want to re-extract archives, or extract archives on top a directory structure that already exists. Additionally, it is fairly common to find tar archives containing the "." directory (possibly with a trailing '/'), which were constructed using "tar -cf foo.tar .". cat, tee These utilities read from stdin using fread(3) into a buffer of size BUFSIZ. However, fread will read until it fills up the entire buffer (or hits EOF) before returning, causing noticeable delay when the input comes from other programs or scripts. To demonstrate this problem, compare the output of these commands: for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | cat for i in $(seq 500) ; do printf 0123456789abcdef ; sleep 0.005 ; done | tee I considered fixing this by making the concat function take an fd instead and make a single call to read(2), but this causes problems for sponge, which uses a FILE * obtained from tmpfile(3) as both output and input for concat. We could also use mkstemp(3) to return a file descriptor, and use a FILE * from fdopen for writing, and the file descriptor for reading, but this seems unclean to me. Another option would be to use fgetc and fputc for the concat implementation, and let libc take care of the buffering. I'm not sure if this has any performance implications. Thanks everyone for their work on sbase! It really has come a long way in the last 6 months. pgpDBGzDW2cHX.pgp Description: PGP signature
