3 ms·

"archives that contain filename like ../../../../../../../tmp/test may overwrite arbitrary files on the system" Maybe paths like that and symlinks should be be

"archives that contain filename like ../../../../../../../tmp/test may overwrite arbitrary files on the system" Maybe paths like that and symlinks should be behind flags that are disabled by default? Specially if you are extracting content that you didn't generate.

deleted 5y ago
[deleted]
TheDong 5y ago
gnu tar does disable extracting paths like that by default. You need to explicitly add the '--absolute-names' flag to gnu tar in order to extract such a malicious archive. I think all tar utilities that are in common usage disable extracting those sorts of paths by default. It just happens that the go stdlib includes a tar implementation that was written from scratch (the go authors are allergic to re-using any C code and will NIH as much as they can).. and they probably don't want to fix it now because the go1 promise heavily discourages making changes like that, even if security is an exception to it. As a random data point, since go and rust are often compared, the most popular tar extraction library in rust has a secure default for this: https://docs.rs/tar/0.4.35/tar/struct.Archive.html#method.unpack https://docs.rs/tar/0.4.35/tar/struct.Archive.html#method.un...
- q3k 5y ago
  > It just happens that the go stdlib includes a tar implementation that was written from scratch (the go authors are allergic to re-using any C code and will NIH as much as they can).. and they probably don't want to fix it now because the go1 promise heavily discourages making changes like that, even if security is an exception to it. It's not about NIH, it's about this being a low-level tar parsing library, not a ready-to-use unpack tool or even a library with unpack functionality. I would never expect such a library to do any sort of automagic path mangling for me unless explicitly stated! I see the same behaviour in libarchive (C library), microtar (C library), and calccrypto/tar (C _tool_ that is vulnerable to this). How would have re-using any C code helped here, if the top three (from Google search results) C tar libraries have the exact same behaviour? The same behaviour is in Python's tarfile, too. Plus, even with Go1 stability guarantees, there's nothing preventing the Go standard library from adding a struct method like .SafePath() if it's deemed necessary - standard library types regularly get new fields/methods added to them. > the most popular tar extraction library in rust has a secure default for this ... only on the high-level unpack methods. The low-level unsafe stuff is still there: https://docs.rs/tar/0.4.35/tar/struct.Entry.html https://docs.rs/tar/0.4.35/tar/struct.Entry.html (.path, .unpack). And that's basically the API level which Go's archive/tar provides. If that crate didn't provide unpack functionality, it would have been the exact same kind of footgun.
  - BeefWellington 5y ago
    > It's not about NIH, it's about this being a low-level tar parsing library, not a ready-to-use unpack tool or even a library with unpack functionality. I would never expect such a library to do any sort of automagic path mangling for me unless explicitly stated! I can appreciate this but I disagree; the defaults should be the exact reverse -- do the safest thing by default and people who "know what they're doing" (famous last words IME) should have a path to do the less safe thing. I lean towards having programming languages that are developed with the most mistake-prone 20-40% of programmers in mind, rather than the absolute best software engineers, though I certainly understand the hesitance to have languages come with training wheels by default.
    - q3k 5y ago
      Automagically rewriting/removing some files based on path fragments is not universal safety. There is no safe subset of extracting archives, IME. You might fix this particular bug class (where someone implements a poor file extraction routine), but you can introduce tons of other classes of bugs, like, off the top of my head: a) someone uses this library for validating whether files contained are conforming a given layout, then passes the tarball to tar - differences in behaviour (tar strips leading .., while eg. the Rust module ignores all files with '..', no matter the position in the path) might cause a security issue. b) silent removal of 'invalid' paths might cause unexpected behavior for when these paths actually are valid and expected (you might not have used ../ in tar, but I have); can imagine one hell of a debugging session coming from that one. I can also imagine codebases that filter tars for malware, and if they happen to skip ../-paths (because of the tar parser silently skipping them), they could let through the tarball unaffected, with some files unscanned, but extractable in GNU tar. c) silent rewriting of 'invalid' paths might cause inconsistency between the behavior of different tar libraries, or might cause internal inconsistency due to two 'invalid' paths sanitizing to a single valid path; imagine someone relying on ../-stripping behaviour at some point, then that behavior changing due to switching libraries or a rearrangement of the files in the source tarball. Sanitizing is a complex issue, and any kind of universal logic might actually introduce more bugs than it solves. d) false sense of security for more complex classes of bugs, like extracting a tarball with a symlink pointing outside of the root of the archive. What should the default behaviour be then, and why? Sometimes symlinking to `/etc` is exactly what the user needs, sometimes it's going to cause a bug further down the line. What about relative symlinks? What about cross-filesystem symlinks? What about the setuid bit? If you make users expect the library to do the right thing, it might cause them to think even less about possible security implications of more tricky situations. Writing user-controlled data to the filesystem is _always_ dangerous and there is no right way to do it, it all depends on the context. I agree that the default behaviour of an _extraction_ library should probably be to sanitize paths. However, I cannot agree that this should be the default behavior of a thin file parser library, like archive/tar. The only thing Go should do, IMO, is explicitly state in documentation that acting on user-supplied tarballs is dangerous and great consideration should be taken before blindly extracting files to the local filesystem.

deleted 5y ago

[deleted]

TheDong 5y ago

gnu tar does disable extracting paths like that by default. You need to explicitly add the '--absolute-names' flag to gnu tar in order to extract such a malicious archive. I think all tar utilities that are in common usage disable extracting those sorts of paths by default. It just happens that the go stdlib includes a tar implementation that was written from scratch (the go authors are allergic to re-using any C code and will NIH as much as they can).. and they probably don't want to fix it now because the go1 promise heavily discourages making changes like that, even if security is an exception to it. As a random data point, since go and rust are often compared, the most popular tar extraction library in rust has a secure default for this: https://docs.rs/tar/0.4.35/tar/struct.Archive.html#method.unpack https://docs.rs/tar/0.4.35/tar/struct.Archive.html#method.un...

q3k 5y ago
> It just happens that the go stdlib includes a tar implementation that was written from scratch (the go authors are allergic to re-using any C code and will NIH as much as they can).. and they probably don't want to fix it now because the go1 promise heavily discourages making changes like that, even if security is an exception to it. It's not about NIH, it's about this being a low-level tar parsing library, not a ready-to-use unpack tool or even a library with unpack functionality. I would never expect such a library to do any sort of automagic path mangling for me unless explicitly stated! I see the same behaviour in libarchive (C library), microtar (C library), and calccrypto/tar (C _tool_ that is vulnerable to this). How would have re-using any C code helped here, if the top three (from Google search results) C tar libraries have the exact same behaviour? The same behaviour is in Python's tarfile, too. Plus, even with Go1 stability guarantees, there's nothing preventing the Go standard library from adding a struct method like .SafePath() if it's deemed necessary - standard library types regularly get new fields/methods added to them. > the most popular tar extraction library in rust has a secure default for this ... only on the high-level unpack methods. The low-level unsafe stuff is still there: https://docs.rs/tar/0.4.35/tar/struct.Entry.html https://docs.rs/tar/0.4.35/tar/struct.Entry.html (.path, .unpack). And that's basically the API level which Go's archive/tar provides. If that crate didn't provide unpack functionality, it would have been the exact same kind of footgun.
- BeefWellington 5y ago
  > It's not about NIH, it's about this being a low-level tar parsing library, not a ready-to-use unpack tool or even a library with unpack functionality. I would never expect such a library to do any sort of automagic path mangling for me unless explicitly stated! I can appreciate this but I disagree; the defaults should be the exact reverse -- do the safest thing by default and people who "know what they're doing" (famous last words IME) should have a path to do the less safe thing. I lean towards having programming languages that are developed with the most mistake-prone 20-40% of programmers in mind, rather than the absolute best software engineers, though I certainly understand the hesitance to have languages come with training wheels by default.
  - q3k 5y ago
    Automagically rewriting/removing some files based on path fragments is not universal safety. There is no safe subset of extracting archives, IME. You might fix this particular bug class (where someone implements a poor file extraction routine), but you can introduce tons of other classes of bugs, like, off the top of my head: a) someone uses this library for validating whether files contained are conforming a given layout, then passes the tarball to tar - differences in behaviour (tar strips leading .., while eg. the Rust module ignores all files with '..', no matter the position in the path) might cause a security issue. b) silent removal of 'invalid' paths might cause unexpected behavior for when these paths actually are valid and expected (you might not have used ../ in tar, but I have); can imagine one hell of a debugging session coming from that one. I can also imagine codebases that filter tars for malware, and if they happen to skip ../-paths (because of the tar parser silently skipping them), they could let through the tarball unaffected, with some files unscanned, but extractable in GNU tar. c) silent rewriting of 'invalid' paths might cause inconsistency between the behavior of different tar libraries, or might cause internal inconsistency due to two 'invalid' paths sanitizing to a single valid path; imagine someone relying on ../-stripping behaviour at some point, then that behavior changing due to switching libraries or a rearrangement of the files in the source tarball. Sanitizing is a complex issue, and any kind of universal logic might actually introduce more bugs than it solves. d) false sense of security for more complex classes of bugs, like extracting a tarball with a symlink pointing outside of the root of the archive. What should the default behaviour be then, and why? Sometimes symlinking to `/etc` is exactly what the user needs, sometimes it's going to cause a bug further down the line. What about relative symlinks? What about cross-filesystem symlinks? What about the setuid bit? If you make users expect the library to do the right thing, it might cause them to think even less about possible security implications of more tricky situations. Writing user-controlled data to the filesystem is _always_ dangerous and there is no right way to do it, it all depends on the context. I agree that the default behaviour of an _extraction_ library should probably be to sanitize paths. However, I cannot agree that this should be the default behavior of a thin file parser library, like archive/tar. The only thing Go should do, IMO, is explicitly state in documentation that acting on user-supplied tarballs is dangerous and great consideration should be taken before blindly extracting files to the local filesystem.