28 ms·

Fix the unit test and open a giant hole everywhere

179 points by zdw 5y ago

gtsop 5y ago
I am wondering if in such development settings people have tests that grep the code for unwanted statements. Eg: since you found that big bug now, grep code for system(".*"\+ to warn or prevent shell calls.
- elanning 5y ago
  I wrote a VS code extension and pre-commit hook for this kind of thing: https://github.com/elanning/checkr https://github.com/elanning/checkr It's a nice inbetween solution for quick stuff not worth writing a full static analysis plugin for.
- dbaupp 5y ago
  Tools like semgrep allow catching injection-like issues like that. For instance, just among existing published rules: - shell=True: https://semgrep.dev/r?q=python.lang.security.audit.subprocess-shell-true.subprocess-shell-true https://semgrep.dev/r?q=python.lang.security.audit.subproces... - system-like calls with a non-static argument: https://semgrep.dev/r?q=python.lang.security.audit.dangerous-system-call.dangerous-system-call https://semgrep.dev/r?q=python.lang.security.audit.dangerous... Custom rules can be written for additional patterns of concern (or specific/uncommon/private APIs) too.
mgaunard 5y ago
It's funny how many of the security vulnerabilities you always hear about (including to some degree the recent log4j vulnerability) are to do with incorrectly escaping inputs when generating a string in another language. This is so basic; why do people keep getting it wrong?
- tome 5y ago
  Because most programming languages make it very hard to correctly model data, for example, they lack sum types/discriminated unions.
- tsimionescu 5y ago
  Because it's very very hard to "correctly" escape strings you don't control, and it's probably an undecidable problem in the general case. From my point of view, any API which accepts special strings from users is a massive burden and should be extremely suspect when used in an automatic fashion. Sure, the shell is nice when you're interacting with it, but calling the shell from your own program is crazy. Similarly, the fact that databases don't offer any structured query fomat and insist on plain text SQL (even with the kludge of prepared statements) is a horrible idea, that has bit everyone time and time again.
  - mgaunard 5y ago
    It's not hard at all. The target programming language has a well-defined grammar that says which special characters need to be escaped in strings, and a mechanism to do so, be that in the case of SQL, sh or printf formatters. Not doing it is just plain incompetence.
    - tsimionescu 5y ago
      It's only easy if you want to escape everything. If there is any intention to mix escaped and unescaped text, it becomes much more difficult.
      - mgaunard 5y ago
        No it isn't. You just need to formally specify what you want to happen, then reason as to whether this leads to adequate semantics for the component being built, and iterate until satisfactory semantics are achieved. This just happens to be the definition of programming.
greenyoda 5y ago
> Worse, since this was changed in a common function that all kinds of programs at the company used, it meant that anything that created a directory in that language (with that library) was now vulnerable to this kind of injection. Low-level libraries that are used all over the company should probably have a strict code review process that's overseen by a group of experienced maintainers who have a good knowledge of security, performance, etc.
- easton 5y ago
  At the very least, given this is a bigco with enough technical staff to have shared libraries for filesystem work (so I'm assuming FAANG or FAANG adjacent), one would think there would be a CI check for naughty stuff (like using system(3)) which requires a n+1 level engineer override to actually get committed. There's almost always a better way.
  - Symbiote 5y ago
    > a bigco with enough technical staff to have shared libraries for filesystem work (so I'm assuming FAANG or FAANG adjacent) What? We are 7 people, and have a shared library for filesystem work. (Since Java 8 about half of it is no longer needed, as it's included, and we replace usage with the standard library as we update projects.)
- floatboth 5y ago
  Ideally they should be turned into open source projects. Google's https://abseil.io https://abseil.io and Facebook's https://github.com/facebook/folly https://github.com/facebook/folly come to mind.
  - alecbz 5y ago
    FWIW the libs in abseil existed as internal-only code for a while at Google before they underwent the effort to open source them, and I think it still likely represents just the tip of the iceberg of Google's internal C++ libs.
enb 5y ago
Not sure of the moral here, but this looks like something that could benefit from a peer review process.
- yjftsjthsd-h 5y ago
  Yeah, > That's how it happens: a tiny little change flings the door wide open. Someone solves their own local problem and misses the bigger picture. feels like an opening to say "...which is why we mandate code review to try and prevent situations where a single person's lack of perspective gets shipped." Of course, that's banking on the reviewer(s) having a broader perspective, so it's only a probabilistic mitigation, but it beats nothing.
  - swiftcoder 5y ago
    Unfortunately, most companies use an organisational model where the reviewer comes from the same team as the author, and hence tends to share many of their assumptions about the system under modification. Code review only works well to prevent this sort of thing if you have designated owners for each library/feature, who are on the hook for the robustness/security/privacy thereof, and whose signoff is mandatory.
- hanche 5y ago
  From the post title, I’d say a big part of the moral is that just because the unit test passes, everything is not necessarily okay.
  - enb 5y ago
    From the title, it’s sounds like that it’s implying that because a unit test was fixed, holes opened everywhere. Which was not the case. The problem was lack of review.
- _dain_ 5y ago
  The moral is the importance of language-theoretic security: http://langsec.org/ http://langsec.org/ >LangSec regards the Internet insecurity epidemic as a consequence of ad hoc input handling. LangSec posits that the only path to trustworthy computer software that takes untrusted inputs is treating all valid or expected inputs as a formal language, and the respective input-handling routine as a parser for that language. Only then can any correctness guarantees be assured for the input-handling code. Ambiguity of message/protocol specification is insecurity; ad hoc parsing is an engine of exploitation; overly complex syntax can make judging security properties of input impractical or even undecidable. >LangSec explains why ad hoc "input sanitization", "sanity checking", and other advice to be more careful with inputs is not enough, and why numerous secure programming initiatives have not ended input-driven exploitation. LangSec is also a code and protocol auditing methodology.
onion2k 5y ago
I hate shared code across entire companies. Someone will write a useful package, it'll be integrated into lots of teams code, and then the original package author leaves. No one maintains their package. A year or two later someone wants to change it, but now no one knows where it's used, what the impact of the change will be, or even whose responsibility it is to manage and maintain the code. All shared code should have a maintainer, and the maintainer should be responsible for updates. If that person leaves then someone else should have to take over that responsibility. In the case of the problem in the article it should be a very simple matter of raising a high priority bug ticket to fix the issue, and the maintainer then informing users of the library that they need to update their dependencies. It never works like that though.
- galaxyLogic 5y ago
  > All shared code should have a maintainer, All code should have a maintainer.
  - rurban 5y ago
    All shared code should have proper documentation (including which errors are thrown, plus unit-tests). Then every flyby can maintain it. A maintainer per se does not help at all, if it's a bad one. Most are bad
    - galaxyLogic 5y ago
      What I'm thinking is that you need code/module ownership. Every module should have an owner, who is its maintainer. Only that way maintainers get credit for doing great maintenance including providing good documentation. If everybody can "maintain" a piece of code nobody is really responsible for it and nobody really gets credit or blame because everybody does.
- mgaunard 5y ago
  I've worked with team leads who were against the idea of assigning maintainers/owners to specific packages, on the rationale that we didn't have enough people to do that (I still do not understand how that rationale works either). Instead you end up in a situation where everyone is responsible for everything, which also means everyone is responsible for nothing.
yjftsjthsd-h 5y ago
So the funny thing is, when I saw that problem description, my first thought was: "oh, that's what mkdir -p does; I wonder if it would be easier to just shell out to that than try and implement it yourself". I grant that that implementation is a massive SQL-injection-like vulnerability, but I'm still not 100% convinced that the basic impulse is wrong; I can easily imagine an alternate universe where the second half of this blog post is talking about how someone wrote it themselves and introduced some subtle bug that the coreutils folks fixed 20 years ago and everyone else has to painfully rediscover. I wonder if any of the major implementations are friendly to being used as a library? Obviously then you have to deal with licensing issues, which is a catch. Maybe fork+exec? I dunno, I'm just trying to feel out whether there's a middle ground between system() and rewriting functionality that someone else has written and that's been used by a billion other users for decades.
- shadowgovt 5y ago
  Yep. Really, the meta-problem here is that someone would want to go run a subprocess because the standard library isn't powerful enough.
- bombcar 5y ago
  You can do something akin to parameterizing SQL with fork and exec (directly setting the arguments) but at that point you might as well call whatever mkdir is doing.
- infotogivenm 5y ago
  There’s absolutely a middle ground, and you are right: it is fork and exec with discrete arguments. It is the intermediary subshell in system() that is the root of all evil, not composing unix programs together; that is what our OS forefathers intended.
  - theptip 5y ago
    I agree. Starting a subprocess is fine. Using an existing tool (mkdir) was probably a better approach than reimplementing it. The error: you should never pass unescaped/untrusted input into a subshell. In Python, it’s the difference between the safe: def make_dir(path): subprocess.run([“mkdir”, “-p”, path]) Instead of: def make_dir_unsafe(path): subprocess.run(“mkdir -p “ + path, shell=True) The latter is easier to type because you don’t need to use the annoying list syntax, but it’s unsafe. If you are writing a library function a subshell is not a sound approach.
nerdponx 5y ago
I know the shell injection thing is bad, but would it be so bad to spawn a /bin/mkdir subprocess without a shell?
- tyingq 5y ago
  There's not much context in the story, but I would guess you might want some sanitizing for stuff like ../../../<whatever>/
  - scwoodal 5y ago
    I prefer the code do any generating of file/directory names. Too many edge cases with user input when working at the OS level for me to sleep at night. Leverage a lookup that says user input “../../../root” == code generated directory path.
- infotogivenm 5y ago
  No, not really. You might need some additional checks, like for ../ and symlinks, but you need these regardless whether you execve or write your own mkdirp().
- gsliepen 5y ago
  Less bad but still bad. Consider that /bin/mkdir is still interpreting whatever you pass to it. What if you provide it a "filename" that starts with a dash? Then /bin/mkdir will interpret that as an option. What if I give it "--version" as the name? It will return 0 (success), even though no directory was created. While it is rather harmless for /bin/mkdir, this might be a bigger problem for other programs.
kgeist 5y ago
>it wasn't just a simple passthrough to the usual C library mkdir() function Aren't standard I/O functions also vulnerable if you don't sanitize your inputs? You can overwrite data and do all sorts of stuff. Sure forking a subprocess is more dangerous, but I think the real issue here is lack of input sanitization when calling OS functions, not the system() call per se.
- hanche 5y ago
  For sure, in the case of mkdir() and fopen() you need to watch out for relative pathnames containing “../“. Is that the sort of thing you had in mind?
  - christophilus 5y ago
    And semicolons, maybe other things: “myfanciuserinput.mp3 && pwned > /etc/hosts”
    - greenyoda 5y ago
      That syntax is interpreted by the shell. It means nothing to mkdir() (an OS-level API) or fopen() (a C library call).
boxed 5y ago
Another lesson is: Don't cripple your standard library. If C had a function for this super common need then this problem would never happen.
- joshuaissac 5y ago
  The C standard does not even have the plain mkdir function. It is defined in POSIX.
- unbanned 5y ago
  Does this make sense? Wouldn't the language spec then need to be aware of what OS it was running on. Or the OS would have to conform to the C specification? Would be a bit weird there
  - boxed 5y ago
    We are talking about creating directories here...
- smarx007 5y ago
  How would it work on a microcontroller without a filesystem?
  - im3w1l 5y ago
    How would fopen work without a filesystem? How would printf work if there is no notion of stdout? C standard has a carveout for this case, in free-standing mode, only a small subset of the standard library is available.
drewcoo 5y ago
Checking for file existence or access), however it is done, often leads to subtle race conditions. Subtle is a word strongly suggesting unchecked assumptions. Bugs come from unchecked assumptions. Generally, just act on the file system. Expect errors to happen and handle them. Even if it's potentially slower. Unless, and in fact I'll say only if, your company is a hot startup revolutionizing file system semantics. More generally, do not be clever about solving problems outside your bailiwick. Unless you want to blog about it and misattribute blame to something like fixing unit tests. (Aside: sounds like the unit tests also had unchecked assumptions.)
- samus 5y ago
  Agree with the first part. However, people willing to look outside of their immediate surroundings a little bit are the counterpart to other people leaving behind such land mines.
10000truths 5y ago
A bit pedantic, but the methods detailed in the blog are vulnerable to time-of-check-to-time-of-use race conditions. The right way to go about this is to use file descriptors wherever possible, so that a stable reference to each intermediate directory is maintained. The openat() and mkdirat() functions should be the go-to for opening and creating intermediate directories.
- kazinator 5y ago
  The main purpose of openat() is to plug the hole that the chdir() current working directory in POSIX is process-wide. So if one thread is calling chdir(), it screws with any other thread that uses traditional Unix calls like open() on a relative path. It's really a ham-fisted band-aid to an poorly designed threading system. The whole thing is in incredibly bad taste. If I'm writing traditional, clean, single-threaded Unix, I don't want anything to do with any at() stuff.
  - pferde 5y ago
    Be that as it may, openat/mkdirat is still useful if one wants to avoid TOCTOU, just like grandparent post suggests.
  - floatboth 5y ago
    But conveniently the at() stuff has allowed for capabilities-based security with Capsicum, CloudABI (RIP) and now WASI. Since the at() functions exist, these environments can just not have a global filesystem namespace – there is no open(), only openat() under the descriptors you have. No escaping up from the sandbox!
    - singron 5y ago
      Can't you openat(fd,"..")? I don't think this is a sufficient sandbox.
      - floatboth 5y ago
        No, not in a capability-based environment. Once you cap_enter() on FreeBSD, you can't – that's the whole point. Same in proper WASI runtime implementations. Please don't assume people are stupid ;) These sandboxes have been carefully designed and extensively tested.
yardstick 5y ago
They needed more unit tests to cover bad cases like .. and ; and other special characters/sequences that shouldn’t be allowed. Not just the happy path.
- fock 5y ago
  The biggest question is: everyone knows "system" is usually not the brightest idea. So why the hell is no pre-merge-hook running to check for this in these companies byzantine monorepos with a lot of unit-tests...
brazzy 5y ago
> That's how it happens: a tiny little change flings the door wide open. Someone solves their own local problem and misses the bigger picture. Just in case anyone missed it: that's pretty much exactly the same thing that happened with the recent Log4Shell disaster. Someone wanted to have a config value from JNDI in their logs and thought it would be cool to have log4j's string interpolation support that directly, found that it was easy to implement, and submitted a patch, which was accepted without any discussion.
mabbo 5y ago
I have a simple rule for shared libraries: only write them for things that are hard. Specialized algorithms; time/date calculations; complex custom business logic shared across the company; code generation libraries that generate code following best practices. These aren't always easy to get right. But left padding strings or creating directories? These are less than 20 lines of code. The cost of using a library does not outweigh the risks when the benefit is that low.
- cowsandmilk 5y ago
  This is an item that looks easy, but lots of people get wrong. Which is why a ton of languages now build it into their standard libraries…
- alecbz 5y ago
  Lines of code != difficulty. There's tons and tons of different possible <20 line implementations for creating a directory + parents, many of which likely have subtle (or not-so-subtle) bugs.
JulianMorrison 5y ago
Using built-in libraries or using the array form of exec to call mkdir or writing your own directory creator feels like it's missing the nub of the problem. You are accepting unsafe input, and trying to work around that without making the input safe. What you should be doing: taking the raw path string and parsing it. Defining via the parser what the form of a permitted path is. Either the input data matches this pattern, in which case it almost doesn't matter how you turn it into directories, or it does not, in which case you can reject it as garbage.
- alecbz 5y ago
  Is there any language with a type system that enforces that possibly user-provided strings/data are tagged as such and forced to be sanitized before use in potentially dangerous functions? This seems like the kind of thing that if done well, could eliminate entire classes of bugs without being too burdensome.
  - joshuamorton 5y ago
    A lot of (most?) type systems can enforce this, but don't by default. You'd define a class "SanitizedStr" (or in this particular case "Path") and have all path operations work on that. Python's pathlib sort of does this, although there's still attacks possible with stuff like joining with an absolute path.
  - blandflakes 5y ago
    Not particularly mainstream, but I remember Ballerina having this: https://ballerina.io/1.0/learn/by-example/taint-checking.html https://ballerina.io/1.0/learn/by-example/taint-checking.htm...
  - JulianMorrison 5y ago
    Taint mode in Perl.
- 2OEH8eoCRo0 5y ago
  This is my instinct working with Perl. Test the input with a regex and move on.
tome 5y ago
The moral of the story is that dealing with the filesystem is like dealing with the reactor room at Chernobyl. Stay away from it unless there is no other option. Instead use a decent database with simple transactional and addressing semantics.
- alecbz 5y ago
  Not really at all the lesson IMO? You could replace the call to `mkdir` with a `psql` call and have the same security hole. The primary issue is with using a shell/subprocess. Maybe you could make the case that "there tend to be better in-language libs for dealing with databases than filesystems". Not sure that's true, even if it is it's sort of a secondary lesson.
alecbz 5y ago
I'm confused: what did the `create_a_directory` helper do before it shelled out to `mkdir -p`? Just a trivial wrapper around the `mkdir` C function?
commandlinefan 5y ago
> unit test that called that directory creation function with a complicated path Unit tests shouldn’t be creating directories in the first place - any outside interaction ought to be mocked out anyway.
- ac2u 5y ago
  Nitpicking. Even if it were misplaced in a unit test, in order to ensure it's working and that you're not hampered by file/folder/permission problems, then there'd be some sort of automated integration test where it does belong, and the story doesn't change one bit.
  - commandlinefan 5y ago
    Yes, I’m changing the subject a bit/focusing on an irrelevant (to the story, important otherwise) detail.
WA 5y ago
I almost expected that some other part of the program used create_directory to actually execute shell commands on purpose, because someone figured this out. Fortunately, that was not the case :)

179 points by zdw 5y ago

I am wondering if in such development settings people have tests that grep the code for unwanted statements. Eg: since you found that big bug now, grep code for system(".*"\+ to warn or prevent shell calls.

elanning 5y ago
I wrote a VS code extension and pre-commit hook for this kind of thing: https://github.com/elanning/checkr https://github.com/elanning/checkr It's a nice inbetween solution for quick stuff not worth writing a full static analysis plugin for.
dbaupp 5y ago
Tools like semgrep allow catching injection-like issues like that. For instance, just among existing published rules: - shell=True: https://semgrep.dev/r?q=python.lang.security.audit.subprocess-shell-true.subprocess-shell-true https://semgrep.dev/r?q=python.lang.security.audit.subproces... - system-like calls with a non-static argument: https://semgrep.dev/r?q=python.lang.security.audit.dangerous-system-call.dangerous-system-call https://semgrep.dev/r?q=python.lang.security.audit.dangerous... Custom rules can be written for additional patterns of concern (or specific/uncommon/private APIs) too.

mgaunard 5y ago

It's funny how many of the security vulnerabilities you always hear about (including to some degree the recent log4j vulnerability) are to do with incorrectly escaping inputs when generating a string in another language. This is so basic; why do people keep getting it wrong?

tome 5y ago
Because most programming languages make it very hard to correctly model data, for example, they lack sum types/discriminated unions.
tsimionescu 5y ago
Because it's very very hard to "correctly" escape strings you don't control, and it's probably an undecidable problem in the general case. From my point of view, any API which accepts special strings from users is a massive burden and should be extremely suspect when used in an automatic fashion. Sure, the shell is nice when you're interacting with it, but calling the shell from your own program is crazy. Similarly, the fact that databases don't offer any structured query fomat and insist on plain text SQL (even with the kludge of prepared statements) is a horrible idea, that has bit everyone time and time again.
- mgaunard 5y ago
  It's not hard at all. The target programming language has a well-defined grammar that says which special characters need to be escaped in strings, and a mechanism to do so, be that in the case of SQL, sh or printf formatters. Not doing it is just plain incompetence.
  - tsimionescu 5y ago
    It's only easy if you want to escape everything. If there is any intention to mix escaped and unescaped text, it becomes much more difficult.
    - mgaunard 5y ago
      No it isn't. You just need to formally specify what you want to happen, then reason as to whether this leads to adequate semantics for the component being built, and iterate until satisfactory semantics are achieved. This just happens to be the definition of programming.

greenyoda 5y ago

> Worse, since this was changed in a common function that all kinds of programs at the company used, it meant that anything that created a directory in that language (with that library) was now vulnerable to this kind of injection. Low-level libraries that are used all over the company should probably have a strict code review process that's overseen by a group of experienced maintainers who have a good knowledge of security, performance, etc.

easton 5y ago
At the very least, given this is a bigco with enough technical staff to have shared libraries for filesystem work (so I'm assuming FAANG or FAANG adjacent), one would think there would be a CI check for naughty stuff (like using system(3)) which requires a n+1 level engineer override to actually get committed. There's almost always a better way.
- Symbiote 5y ago
  > a bigco with enough technical staff to have shared libraries for filesystem work (so I'm assuming FAANG or FAANG adjacent) What? We are 7 people, and have a shared library for filesystem work. (Since Java 8 about half of it is no longer needed, as it's included, and we replace usage with the standard library as we update projects.)
floatboth 5y ago
Ideally they should be turned into open source projects. Google's https://abseil.io https://abseil.io and Facebook's https://github.com/facebook/folly https://github.com/facebook/folly come to mind.
- alecbz 5y ago
  FWIW the libs in abseil existed as internal-only code for a while at Google before they underwent the effort to open source them, and I think it still likely represents just the tip of the iceberg of Google's internal C++ libs.

enb 5y ago

Not sure of the moral here, but this looks like something that could benefit from a peer review process.

yjftsjthsd-h 5y ago
Yeah, > That's how it happens: a tiny little change flings the door wide open. Someone solves their own local problem and misses the bigger picture. feels like an opening to say "...which is why we mandate code review to try and prevent situations where a single person's lack of perspective gets shipped." Of course, that's banking on the reviewer(s) having a broader perspective, so it's only a probabilistic mitigation, but it beats nothing.
- swiftcoder 5y ago
  Unfortunately, most companies use an organisational model where the reviewer comes from the same team as the author, and hence tends to share many of their assumptions about the system under modification. Code review only works well to prevent this sort of thing if you have designated owners for each library/feature, who are on the hook for the robustness/security/privacy thereof, and whose signoff is mandatory.
hanche 5y ago
From the post title, I’d say a big part of the moral is that just because the unit test passes, everything is not necessarily okay.
- enb 5y ago
  From the title, it’s sounds like that it’s implying that because a unit test was fixed, holes opened everywhere. Which was not the case. The problem was lack of review.
_dain_ 5y ago
The moral is the importance of language-theoretic security: http://langsec.org/ http://langsec.org/ >LangSec regards the Internet insecurity epidemic as a consequence of ad hoc input handling. LangSec posits that the only path to trustworthy computer software that takes untrusted inputs is treating all valid or expected inputs as a formal language, and the respective input-handling routine as a parser for that language. Only then can any correctness guarantees be assured for the input-handling code. Ambiguity of message/protocol specification is insecurity; ad hoc parsing is an engine of exploitation; overly complex syntax can make judging security properties of input impractical or even undecidable. >LangSec explains why ad hoc "input sanitization", "sanity checking", and other advice to be more careful with inputs is not enough, and why numerous secure programming initiatives have not ended input-driven exploitation. LangSec is also a code and protocol auditing methodology.

onion2k 5y ago

I hate shared code across entire companies. Someone will write a useful package, it'll be integrated into lots of teams code, and then the original package author leaves. No one maintains their package. A year or two later someone wants to change it, but now no one knows where it's used, what the impact of the change will be, or even whose responsibility it is to manage and maintain the code. All shared code should have a maintainer, and the maintainer should be responsible for updates. If that person leaves then someone else should have to take over that responsibility. In the case of the problem in the article it should be a very simple matter of raising a high priority bug ticket to fix the issue, and the maintainer then informing users of the library that they need to update their dependencies. It never works like that though.

galaxyLogic 5y ago
> All shared code should have a maintainer, All code should have a maintainer.
- rurban 5y ago
  All shared code should have proper documentation (including which errors are thrown, plus unit-tests). Then every flyby can maintain it. A maintainer per se does not help at all, if it's a bad one. Most are bad
  - galaxyLogic 5y ago
    What I'm thinking is that you need code/module ownership. Every module should have an owner, who is its maintainer. Only that way maintainers get credit for doing great maintenance including providing good documentation. If everybody can "maintain" a piece of code nobody is really responsible for it and nobody really gets credit or blame because everybody does.
mgaunard 5y ago
I've worked with team leads who were against the idea of assigning maintainers/owners to specific packages, on the rationale that we didn't have enough people to do that (I still do not understand how that rationale works either). Instead you end up in a situation where everyone is responsible for everything, which also means everyone is responsible for nothing.

yjftsjthsd-h 5y ago

So the funny thing is, when I saw that problem description, my first thought was: "oh, that's what mkdir -p does; I wonder if it would be easier to just shell out to that than try and implement it yourself". I grant that that implementation is a massive SQL-injection-like vulnerability, but I'm still not 100% convinced that the basic impulse is wrong; I can easily imagine an alternate universe where the second half of this blog post is talking about how someone wrote it themselves and introduced some subtle bug that the coreutils folks fixed 20 years ago and everyone else has to painfully rediscover. I wonder if any of the major implementations are friendly to being used as a library? Obviously then you have to deal with licensing issues, which is a catch. Maybe fork+exec? I dunno, I'm just trying to feel out whether there's a middle ground between system() and rewriting functionality that someone else has written and that's been used by a billion other users for decades.

shadowgovt 5y ago
Yep. Really, the meta-problem here is that someone would want to go run a subprocess because the standard library isn't powerful enough.
bombcar 5y ago
You can do something akin to parameterizing SQL with fork and exec (directly setting the arguments) but at that point you might as well call whatever mkdir is doing.
infotogivenm 5y ago
There’s absolutely a middle ground, and you are right: it is fork and exec with discrete arguments. It is the intermediary subshell in system() that is the root of all evil, not composing unix programs together; that is what our OS forefathers intended.
- theptip 5y ago
  I agree. Starting a subprocess is fine. Using an existing tool (mkdir) was probably a better approach than reimplementing it. The error: you should never pass unescaped/untrusted input into a subshell. In Python, it’s the difference between the safe: def make_dir(path): subprocess.run([“mkdir”, “-p”, path]) Instead of: def make_dir_unsafe(path): subprocess.run(“mkdir -p “ + path, shell=True) The latter is easier to type because you don’t need to use the annoying list syntax, but it’s unsafe. If you are writing a library function a subshell is not a sound approach.

nerdponx 5y ago

I know the shell injection thing is bad, but would it be so bad to spawn a /bin/mkdir subprocess without a shell?

tyingq 5y ago
There's not much context in the story, but I would guess you might want some sanitizing for stuff like ../../../<whatever>/
- scwoodal 5y ago
  I prefer the code do any generating of file/directory names. Too many edge cases with user input when working at the OS level for me to sleep at night. Leverage a lookup that says user input “../../../root” == code generated directory path.
infotogivenm 5y ago
No, not really. You might need some additional checks, like for ../ and symlinks, but you need these regardless whether you execve or write your own mkdirp().
gsliepen 5y ago
Less bad but still bad. Consider that /bin/mkdir is still interpreting whatever you pass to it. What if you provide it a "filename" that starts with a dash? Then /bin/mkdir will interpret that as an option. What if I give it "--version" as the name? It will return 0 (success), even though no directory was created. While it is rather harmless for /bin/mkdir, this might be a bigger problem for other programs.

kgeist 5y ago

>it wasn't just a simple passthrough to the usual C library mkdir() function Aren't standard I/O functions also vulnerable if you don't sanitize your inputs? You can overwrite data and do all sorts of stuff. Sure forking a subprocess is more dangerous, but I think the real issue here is lack of input sanitization when calling OS functions, not the system() call per se.

hanche 5y ago
For sure, in the case of mkdir() and fopen() you need to watch out for relative pathnames containing “../“. Is that the sort of thing you had in mind?
- christophilus 5y ago
  And semicolons, maybe other things: “myfanciuserinput.mp3 && pwned > /etc/hosts”
  - greenyoda 5y ago
    That syntax is interpreted by the shell. It means nothing to mkdir() (an OS-level API) or fopen() (a C library call).

boxed 5y ago

Another lesson is: Don't cripple your standard library. If C had a function for this super common need then this problem would never happen.

joshuaissac 5y ago
The C standard does not even have the plain mkdir function. It is defined in POSIX.
unbanned 5y ago
Does this make sense? Wouldn't the language spec then need to be aware of what OS it was running on. Or the OS would have to conform to the C specification? Would be a bit weird there
- boxed 5y ago
  We are talking about creating directories here...
smarx007 5y ago
How would it work on a microcontroller without a filesystem?
- im3w1l 5y ago
  How would fopen work without a filesystem? How would printf work if there is no notion of stdout? C standard has a carveout for this case, in free-standing mode, only a small subset of the standard library is available.

drewcoo 5y ago

Checking for file existence or access), however it is done, often leads to subtle race conditions. Subtle is a word strongly suggesting unchecked assumptions. Bugs come from unchecked assumptions. Generally, just act on the file system. Expect errors to happen and handle them. Even if it's potentially slower. Unless, and in fact I'll say only if, your company is a hot startup revolutionizing file system semantics. More generally, do not be clever about solving problems outside your bailiwick. Unless you want to blog about it and misattribute blame to something like fixing unit tests. (Aside: sounds like the unit tests also had unchecked assumptions.)

samus 5y ago
Agree with the first part. However, people willing to look outside of their immediate surroundings a little bit are the counterpart to other people leaving behind such land mines.

10000truths 5y ago

A bit pedantic, but the methods detailed in the blog are vulnerable to time-of-check-to-time-of-use race conditions. The right way to go about this is to use file descriptors wherever possible, so that a stable reference to each intermediate directory is maintained. The openat() and mkdirat() functions should be the go-to for opening and creating intermediate directories.

kazinator 5y ago
The main purpose of openat() is to plug the hole that the chdir() current working directory in POSIX is process-wide. So if one thread is calling chdir(), it screws with any other thread that uses traditional Unix calls like open() on a relative path. It's really a ham-fisted band-aid to an poorly designed threading system. The whole thing is in incredibly bad taste. If I'm writing traditional, clean, single-threaded Unix, I don't want anything to do with any at() stuff.
- pferde 5y ago
  Be that as it may, openat/mkdirat is still useful if one wants to avoid TOCTOU, just like grandparent post suggests.
- floatboth 5y ago
  But conveniently the at() stuff has allowed for capabilities-based security with Capsicum, CloudABI (RIP) and now WASI. Since the at() functions exist, these environments can just not have a global filesystem namespace – there is no open(), only openat() under the descriptors you have. No escaping up from the sandbox!
  - singron 5y ago
    Can't you openat(fd,"..")? I don't think this is a sufficient sandbox.
    - floatboth 5y ago
      No, not in a capability-based environment. Once you cap_enter() on FreeBSD, you can't – that's the whole point. Same in proper WASI runtime implementations. Please don't assume people are stupid ;) These sandboxes have been carefully designed and extensively tested.

yardstick 5y ago

They needed more unit tests to cover bad cases like .. and ; and other special characters/sequences that shouldn’t be allowed. Not just the happy path.

fock 5y ago
The biggest question is: everyone knows "system" is usually not the brightest idea. So why the hell is no pre-merge-hook running to check for this in these companies byzantine monorepos with a lot of unit-tests...

brazzy 5y ago

> That's how it happens: a tiny little change flings the door wide open. Someone solves their own local problem and misses the bigger picture. Just in case anyone missed it: that's pretty much exactly the same thing that happened with the recent Log4Shell disaster. Someone wanted to have a config value from JNDI in their logs and thought it would be cool to have log4j's string interpolation support that directly, found that it was easy to implement, and submitted a patch, which was accepted without any discussion.

mabbo 5y ago

I have a simple rule for shared libraries: only write them for things that are hard. Specialized algorithms; time/date calculations; complex custom business logic shared across the company; code generation libraries that generate code following best practices. These aren't always easy to get right. But left padding strings or creating directories? These are less than 20 lines of code. The cost of using a library does not outweigh the risks when the benefit is that low.

cowsandmilk 5y ago
This is an item that looks easy, but lots of people get wrong. Which is why a ton of languages now build it into their standard libraries…
alecbz 5y ago
Lines of code != difficulty. There's tons and tons of different possible <20 line implementations for creating a directory + parents, many of which likely have subtle (or not-so-subtle) bugs.

JulianMorrison 5y ago

Using built-in libraries or using the array form of exec to call mkdir or writing your own directory creator feels like it's missing the nub of the problem. You are accepting unsafe input, and trying to work around that without making the input safe. What you should be doing: taking the raw path string and parsing it. Defining via the parser what the form of a permitted path is. Either the input data matches this pattern, in which case it almost doesn't matter how you turn it into directories, or it does not, in which case you can reject it as garbage.

alecbz 5y ago
Is there any language with a type system that enforces that possibly user-provided strings/data are tagged as such and forced to be sanitized before use in potentially dangerous functions? This seems like the kind of thing that if done well, could eliminate entire classes of bugs without being too burdensome.
- joshuamorton 5y ago
  A lot of (most?) type systems can enforce this, but don't by default. You'd define a class "SanitizedStr" (or in this particular case "Path") and have all path operations work on that. Python's pathlib sort of does this, although there's still attacks possible with stuff like joining with an absolute path.
- blandflakes 5y ago
  Not particularly mainstream, but I remember Ballerina having this: https://ballerina.io/1.0/learn/by-example/taint-checking.html https://ballerina.io/1.0/learn/by-example/taint-checking.htm...
- JulianMorrison 5y ago
  Taint mode in Perl.
2OEH8eoCRo0 5y ago
This is my instinct working with Perl. Test the input with a regex and move on.

tome 5y ago

The moral of the story is that dealing with the filesystem is like dealing with the reactor room at Chernobyl. Stay away from it unless there is no other option. Instead use a decent database with simple transactional and addressing semantics.

alecbz 5y ago
Not really at all the lesson IMO? You could replace the call to `mkdir` with a `psql` call and have the same security hole. The primary issue is with using a shell/subprocess. Maybe you could make the case that "there tend to be better in-language libs for dealing with databases than filesystems". Not sure that's true, even if it is it's sort of a secondary lesson.

alecbz 5y ago

I'm confused: what did the `create_a_directory` helper do before it shelled out to `mkdir -p`? Just a trivial wrapper around the `mkdir` C function?

commandlinefan 5y ago

> unit test that called that directory creation function with a complicated path Unit tests shouldn’t be creating directories in the first place - any outside interaction ought to be mocked out anyway.

ac2u 5y ago
Nitpicking. Even if it were misplaced in a unit test, in order to ensure it's working and that you're not hampered by file/folder/permission problems, then there'd be some sort of automated integration test where it does belong, and the story doesn't change one bit.
- commandlinefan 5y ago
  Yes, I’m changing the subject a bit/focusing on an irrelevant (to the story, important otherwise) detail.

WA 5y ago

I almost expected that some other part of the program used create_directory to actually execute shell commands on purpose, because someone figured this out. Fortunately, that was not the case :)