Fix the unit test and open a giant hole everywhere | Hacker News Reader

Fix the unit test and open a giant hole everywhere

rachelbythebay.com

179 pointszdw157 comments

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.

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.

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.

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.

I like to do this to avoid list syntax:

    def make_dir(path):
      subprocess.run('mkdir -p'.split() + [path])

So you like typing more and making your program harder to read to avoid using two apostrophes and two commas.

If you have a command with lots of arguments, parent’s approach makes a lot of sense. 20 arguments that you can copy out of the shell into your program. And when you want to make changes and test via the shell, your copy and paste is also easy.

I’ve seen bugs where some forgot to add quotes and commas to split some arguments and the program did the wrong thing. Parent’s approach avoids that. However, things like spaces in quoted strings are not correctly handled.

I am not exactly sure if parent's code execute the way you think it does. path is provided as an argument to mkdir -p, even if you set path to be a string with 20 arguments copied from your shell, it'll get passed as one.

I assume you meant something like subprocess.run(['mkdir', '-p', '--', *path.split()]), which in that case using shlex.split will fix your problems with quoted strings.

Naively splitting commands using split() will bite you in the ass eventually. Use shlex.split as the other comment says, or just stop being lazy and craft the argument list carefully, once, and be done with it. Shortcuts here aren’t worth the risk.

You shouldn't pass unescaped/untrusted input into a subshell but your suggestion doesn't solve for every case. It protects against ";" but not path traversal with "../../../../../foo/bar".

You'd have to cover that if you wrote your own library function, though; it's attack surface, but not uniquely so.

    subprocess.run([“mkdir”, “-p”, path])

This does not work for all values of path. And I'm not just referring to blorgle's complaint. I'm being deliberately vague to make you pause for a second and think.

Does `path = "$(rm -rf /)"` work in this context?

No, because this isn’t being run through the shell, it’s directly exec’ing the program. So you’re just going to get a dir named “$(rm -rf /)”.

It's funny how easy it is to miss characters with special meaning. In this case it's the slash. You would get a directory named "$(rm -rf " with a subdirectory named ")"

You seem to be getting downvoted for being too vague, so I will post the solution:

The snippet breaks for paths starting with a minus, as mkdir will interpret those as another option.

The solution is:

    subprocess.run(["mkdir", "-p", "--", path])

Hmmm. In Python, I would leverage the dedicated utility of the standard library:

  os.makedirs(path)

Yes, agreed. I was using Python code to more explicitly illustrate the subprocess vs shell distinction in the general case, not to provide a solution to the specific problem of making a directory.

Doing fork/exec for the mkdir tool is still many times slower that calling a good library function.

> Doing fork/exec for the mkdir tool is still many times slower that calling a good library function.

Would it be noticeably slower to the point it's measurably relevant, though? Or are we talking about something well within the domain of premature optimization?

I mean,if you're doing IO, how often does creating a dir end up being the performance bottleneck?

Premature optimization is a common disease amongst C and C++ devs. I often come across code that is written in an obscure/complex way just because someone thought execution speed triumphs code readability. In most cases (around 9 times out of 10) it is on a non-time-critical path of the code and the developer who wrote it didn't even bother to run a profiler on it. My work would be significantly easier if we could learn to prioritize code readability.

The point of those languages is to make it explicit when you're doing ridiculously costly things so that you're forced to do a reasonable implementation instead.

I disagree that C++ makes expensive operations explicit; see the copy constructor and the copy assignment operators. C, maybe.

A C++ developer would know what an assignment or copy entails and would spot them on sight.

It's impossible to tell whether a specific line of code makes a copy without knowing the types of all values. i.e.

    std::vector<int> numbers = Func();

may or may not make a copy depending on the signature of Func. So it's not clear at the callsite how expensive that line of code is. This is unlike Go (which has this explicitly as philosophy) where you need to call a copy function/use append to make a copy of a slice.

Knowing the declarations of the various names a piece of code uses is of course a requirement in order to understand what it does.

In that example, there would always be a move or copy there, though there are scenarios in which it might be elided.

No, any language that allows operator overloading can hide ridiculously costly operations behind mundane-looking code. C is pretty good about that, but C++ is not: you can even overload the comma operator!

Consider a + b + c.

When you read the code, you know what the types of a, b and c are. You know whether they're built-in types, and therefore whether those operator expressions are actually function calls.

Nothing is hidden, so long as the programmer doesn't make invalid assumptions based on his experience with other, different languages.

Your point appears to be "if you know the entire codebase, nothing is hidden." This is tautologically true of all languages. If you're accustomed to working on small projects, then that attitude will work out for you. If you work on large codebases, it isn't always so easy, especially if folks use a lot of template magic.

In your case, if a, b, and c are all the same type, then b+c can return a completely different type, and you'll need to track that down before you know what type a+(b+c) will have. And in that case, (a+b)+c can have a different type from a+(b+c). If you've memorized the associativity rules for the language, you'll know what to expect -- but it's not at all obvious from the notation.

And, sure. If you know absolutely everything about the language and everything about the codebase, you can make accurate predictions about the behavior of any given line. But if you're coming into a new codebase, there's no way to know at a glance what a given line of code is going to cost. That's what folks are calling hidden complexity.

Oh -- and thanks to the auto keyword, you don't always know the types of a, b, and c without hunting things down.

You don't need to know the whole codebase to understand a piece of code, just what the declarations of the names being referenced in that piece of code are (in C++, a name may be a macro, a type, a template, a variable or a function, and depending on which kind, may have all kinds of other properties attached).

Yes, you need to look at the declarations of names to know what the resulting type of expressions involving those names is. I don't see that as being surprising or unusual. The same is also true in C.

It depends a lot on how often you’re trying to do it and the conditions you’re doing it under. I’ve worked on a couple of projects where I own an agent process that runs on thousands of machines that are not mine (so I have no control of whether they are overloaded or in some pathological state), some of which are bumping up against memory and/or cpu and or IO limits, and a single blocking system call is many orders of magnitude more practical than the overhead of fork/exec and the dozens or hundreds of blocking system calls and memory allocations that entails.

This is, of course, an extreme example. But there are definitely cases where it matters a lot.

Completely agreed. But likely many many times faster to write. Just depends what youre optimizing for.

> It is the intermediary subshell in system() that is the root of all evil

Certainly the worst of it, but even if you pass the untrusted data as a discrete arguments you're still vulnerable to parameter injection.

Let's say, for example, that you're running ["find", USER_SUPPLIED_PATH, "-type", "f"]. If the user supplies "-delete" as the path, the resulting behaviour would not be as intended.

Very true. A pro tip here is use ‘—-‘ :)

I'd want a better reason before shelling out personally. Apart from security, other issues are:

- performance. Obviously that depends on the circumstances but if you are working on a function that is used in many places you don't know the circumstances.

- error handling and reporting. Are you going to capture stderr from mkdir? If this fails can you provide a clear indication of the problem to the caller?

- paths. If you run mkdir without specifying an absolute path your program may fail if $path is not what you expect. That can also be a vulnerability.

- dependencies. Now you won't run in a distroless container or other cut down environment

- you lose type safety

Not saying "never shell out" but I've been burned by most of these so I am cautious about it.

“Never shell out” is pretty close to correct though. I’d say “only shell out as a last resort, and only after you’ve considered all the drawbacks.”

It seems to me that hiding a risky implementation is the problem. If you were directly shelling out to `mkdir -p` yourself, it would (hopefully) at least be obvious that it's a risky thing to do, and you need to make sure that either the input couldn't possibly contain any data touched by a malicious user or that it was sufficiently well sanitized to protect against that sort of thing. But if it's hidden behind a friendly-look library call, it would be less obvious that it's a risk, and it's easier to see someone absent-mindedly calling it because it's handy and passing in data from an unauthenticated web user or something.

I think the real fix here is to investigate why you're creating possibly-already-existing directories anyway. Most files or directories should either be owned by a specific component (which then creates them and fails if they already exist) or should be assumed to already exist (as a requirement that the environment is set up properly.

It depends upon usage. My blogging software keeps entries in the file system, in a hierarchy as YYYY/MM/DD. I just create the directories as I add entries by doing a "mkdir(year); mkdir(year "/" month); mkdir(year "/" month "/" day)".

Imo the middle ground here is that this is a common enough operation that it should be in your language's standard library, see e.g. [0][1]. And if you have to use C, then yes, write it yourself or copy some other implementation with a compatible license. That's what you signed up for when you decided you need C.

[0] https://pkg.go.dev/os#MkdirAll

[1] https://doc.rust-lang.org/std/fs/fn.create_dir_all.html

> Imo the middle ground here is that this is a common enough operation that it should be in your language's standard library, see e.g. [0][1].

Is it, though?

I mean, C is around for around half a century, and C is perhaps the primary language in the FLOSS universe.

Still, why is there no widely adopted idempotent directory tree creation library?

Because C stopped evolving long ago, in spite of the recent C standards that have more to do with a reanimated zombie than anything else.

In C++ that functionality is indeed part of the standard library.

> Because C stopped evolving long ago (...)

That excuse isn't credible. I mean, I asked about a libraries that handled that usecase, not a standard library. If there was demand for that use case, and that demand popped up so often that it justified the adoption of a de-facto standard, why isn't there any third-party library specialized in that usecase? I mean, putting together a library takes far less work than presenting a proposal to the C standardization committee, which is open to proposals.

So, why isn't there one? Might it be that there is actually no relevant demand for it?

How would such a trivial library achieve popularity? It would have to be part of a larger framework.

Anyway I just checked and Glib does seem to have it for example.

> How would such a trivial library achieve popularity?

There is no size limit on how many features could ship with a library. In fact, we live in an age where packages like rimraff report around 50 million downloads per week.

But the original question, and the point it made,still stand: why isn't there such a library to provide such a feature? Might it be that you're overblowing it's demand and usefulness? Perhaps there is no such thing in the standard library because no one bothers with it, or feels it's missing?

Libraries are a pain in C. So most C programs use less of them, than if you are writing js

> Libraries are a pain in C.

They really aren't. At all. What exactly are the pain points you experienced with C libraries?

> So most C programs use less of them

This assertion doesn't hold any water. In all the years I worked with C and C++ projects, not even once did I ever heard "let's not add a library, it's hard".

Do you actually have a concrete, real issue in mind?

> What exactly are the pain points you experienced with C libraries?

1. Varying build systems. Does the dependency use cmake? Autoconf? A shell script where you’re instructed to modify some variables specific to your system?

2. The above, but my software is shipping on Linux, windows, and Mac. What’s the best way to get this dependency compiled? Does upstream even support all these OSes?

In all my years of doing C programming, I’ve never seen some add a single function library like is_even and then another like is_odd. But you do see it in languages where it is really easy to add libraries.

in C I always end up building my own world because the default one is broken. for example null terminated strings are just really awful to work with. i'm not alone - alot of larger C projects have their own runtime, particularly memory management. another cross-cutting concern is multihreaded safety and scheduling.

unless the library author broke those functions out, then I need to rehost it in my environment. so I usually end up forking any library that I want to use. this isn't all bad - I find some bugs and integrate the build. I adopt the library and develop and understanding of it sufficient to maintain it myself.

contrast that to a language environment where those things - strings and threads and build are all standardized.

My point of view is that if you program in C you most likely care about being in control of what happens.

Your approach makes perfect sense within that context.

I already gave you an example of a C library that provided that functionality.

And no, people don't reuse trivial libraries in C, this is not JavaScript.

Cross plateform make directory is trivial?

> And no, people don't reuse trivial libraries in C, this is not JavaScript.

You do realize that tribalist arguments based on absolutely no technical point whatsoever do not make any sense at all, don't you? I mean, who in their right mind would argue that only nodejs developers need to delete dirs?

And if you believe that putting together a reliable cross-platform library that handles idempotent directory creation then be my guest and just do it. How hard would it be, right?

This is merely a factual comment.

Node.JS is widely known for having people depend on trivial packages (I vaguely remember some story about half the world being affected because the package doing a trim space went down).

The same is not true for C. Libraries there tend to cover larger domains.

Whether that is caused by technical limitations, culture, or competence of the developers is all irrelevant to that statement.

Let’s not act so high and mighty. It got added in c++17 which is only four years ago and it took most implementations quite a while to implement it in the official <filesystem> location (vs as an experimental header).

Oh. And we’re still lacking any kind of sockets support. There’s this thing I’m hearing about called the Internet that’s going to be big any day now and might need this.

> Oh. And we’re still lacking any kind of sockets support. There’s this thing I’m hearing about called the Internet that’s going to be big any day now and might need this.

Between libcurl, POCO, or even Qt, does it matter at all that the C++ standard offers "any kind of sockets support"?

Just because Java forced everything under the sun into their standard library, that does not mean that move makes any sense at all. Between Boost and Apache and POCO and others like that, it's high time we stop insisting in this nonsense.

That’s one position. The other is one most other languages choose which is some kind of cross platform sockets support out of the box with packages filling in more niche cases for performance or usability reasons.

You could easily make the same counter argument against including <filesystem> and yet there’s very real value to having something cross platform out of the box that handles elegantly various pitfalls that people typically fall into.

> The other is one most other languages choose which is some kind of cross platform sockets support out of the box with packages filling in more niche cases for performance or usability reasons.

Given that Java followed that path and already acknowledged that mistake by deprecating the original HTTP client, why do people insist in not learning lessons?

Because what the standard library offers, is supposed to be available across all platforms and implementations of a specific language, with a certain expectation level in quality.

What is available as external library has various levels of support quality.

That is why we have POSIX, the C runtime that ISO C didn't want to rely on, yet almost every C program uses a part of it.

Pretty sure many or most Windows programs don't use the POSIX library.

Indeed, there is a semantic difference between almost every and all of them.

Still it was deemed important enough to spend development resources on the POSIX subsystem to make DoD happy, then SFU and now integrate the Linux kernel.

I don't think there is a need to bring up "semantics" here, and the only reasonably defined semantics that I know for the term "almost all" would be from set theory where that is a useless term without an infinite set to measure against.

Common sense anyway would suggest that "almost every" is distinct from "many, but also many not", and the latter is what I think would be a better fit here.

The intention behind my comment (and I thought it was clear without any "semantics" discussion required) was to show that there is no need for the POSIX layer to be included as part of ISO C. POSIX is just a "platform layer" (for all I know), meaning that it is much closer to a "library" than a "runtime". I don't see a need to bundle it tightly with the language. Heck, why did I even bring up Windows as a counterexample, when there are probably a lot of more-or-less ISO C conforming compilers for microcontroller platforms that could never support POSIX.

HTTP != sockets. I’m not arguing for HTTP within the standard library but for low-level networking support. That one should be non controversial.

As for HTTP within the standard library, I would probably tend to agree although I’d note that Python does have it in its standard module set and I don’t think there’s an acknowledgement that that’s somehow a mistake. Each language fills a different need so framing it as “people insisting in not learning lessons” isn’t helpful. Not to mention that the best way to learn is to experiment and double check if what was once true remains that way (experts know when and in what way to break rules).

I fail to see how "any kind of sockets support" is true, given that there is a networking TS as well as well-established networking libraries.

The main reason why all these things haven't really been integrated into the main language yet is because the committee is still experimenting with various approaches to asynchronous programming and scheduling with goals of also integrating these with heterogeneous computing.

Also sockets are arguably an outdated networking model anyway, so people really serious about high-performance networking will be doing their own thing regardless.

I could buy that reasoning in the 90s. Heck even in 2000 after the dotcom bubble burst. I could maybe buy it in 2010 although by then networking was in no way even remotely niche. We’re now into the third decade of networking and far less important additions to the standard library have been made.

A TS is almost nothing in the c++ world. Each implementation will have its own quirks and unreliable cross platform behavior if it’s even available. There are well established filesystem libraries too (heck the STL implementation is almost identical to boost for that reason to make it easier for those people to port and for the library maintainers you basically just wholesale copy an implementation or at least have a reference one to test most things against).

> Also sockets are arguably an outdated networking model anyway, so people really serious about high-performance networking will be doing their own thing regardless.

I’m eager to hear what you mean by this because I’m unaware of any OS APIs that do Networking in any other way. Unless you’re talking about userspace drivers or DPDK, neither of which see serious adoption outside of data centers (if even there). 99.99% of an networking code I’ve observed across mobile and cloud is socket-based.

High-performance networking somewhat excludes mobile and cloud.

Regardless, the io_uring interface, which is general-purpose and still based on file descriptors for its initialization, does go out of its way to work around some of the problems with the socket model.

I think the real issue here is the lack of distinction in the rest of the codebase between trusted and untrusted input. There should be only a few, clearly identified functions that you do pass unsanitised user input into.

I think this, really, is the take away. Everyone is getting stuck on the subshell part, but it's the untrusted data part that's the problem.

Especially because the early examples in the article (making subdirs for something like different packages/release - "project/staging", "project/production") are almost certainly safe to use a subshell for - so much so that I was genuinely wondering why the author wouldn't just call out to "mkdir -p" for that use case.

The error came when someone decided to pass untrusted/user-supplied data to this function (the later example - usernames).

But I think doing the later is a huge mistake either way. Don't pass untrusted & unsanitized user data ANYWHERE that isn't explicitly designed for it.

Who gives a hoot whether that's to a subshell, or a pipe/stream, or to a sql query, or your DOM, or any other of hundred of places that user supplied data isn't safe.

I like the idea that Joel had: make untrusted user provided strings into a distinct data type that doesn't easily fit into the mold of a trusted, already-sanitised string type.

If you ever see an unsafe string usage being passed into a function that expects a safe string, without an accompanying call to sanitise(), that's very wrong (and won't work if the language is strongly typed) and should be recognised/corrected as such immediately.

Yeah, I actually nearly put in something about separate trusted vs. untrusted types but didn't want to dilute the main point or hijack it religious wars over typed languages. :P

I think there could be some ways to make it work even in untyped/dynamically typed languages, as long as they support prototypes. Or use Applications Hungarian (not Systems Hungarian!) to add this facet of type information.

Yep. Really, the meta-problem here is that someone would want to go run a subprocess because the standard library isn't powerful enough.

I feel strongly that you shouldn't go looking for external code that does simple things like this. You can reimplement the relevant parts of mkdir -p in 20 lines of C code, and I think that's what you should do if the standard library doesn't have it.

Having the code inline makes it easy to audit, and prevents problems when a dependency changes 5 years later.

It also trains people to write proper C code. Writing loops and checking errno should be second nature for a C programmer, and doing it often helps you learn.

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.

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.

Be that as it may, openat/mkdirat is still useful if one wants to avoid TOCTOU, just like grandparent post suggests.

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!

Can't you openat(fd,"..")? I don't think this is a sufficient sandbox.

openat2 now has RESOLVE_BENEATH to fix that.

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.

Wrong. Read the man page for openat - the main reason for its use is explained and it is not the one you mention, though it is given as a secondary reason.

https://linux.die.net/man/2/openat

Then you are opening yourself up to a world of subtle security holes with symlinks.

The POSIX file system API is a festering sore, an utter disaster. Symlinks are the cause.

Bring back directory hardlinks!

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.)

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.

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.

Everything is a trade off. The other extreme is having many libraries with overlapping concerns where the same security holes show up again and again.

I think the real answer is to decide on a strategy and then put healthy process in place to mitigate the downsides. In the case of sharing code across the company, make sure there are 2+ maintainers. This sort of succession planning is common. It is not foolproof, but it does mitigate the issue you raised.

> no one knows where it's used ...

Isn't this really a tooling problem? I can't imagine any environment in which I can't instantly find all callers/users of a shared package. The problem I always have is that there are too many [known] callers! Impact is still unknown because you can't always depend on unit and other test to uncover complex issues unforeseen and unknown until they actually crop up. But that's why you have a staging environment.

It's better than the alternative of not sharing code.

> I can't imagine any environment in which I can't instantly find all callers/users of a shared package

All the people who spent that last week or two hunting down places where log4j got imported by dependencies like Springboot or Grails are crying into their Xmas whisky at this…

Had that job, and I disagree. Maven Dependency plugin has got you there. `dependency:tree` even shows you which library brought it along. The difficult part was how to make dependency versions from `log4j-bom` win over the `spring-boot-dependencies` BOM. Also, one or two places in the code actually used some utility functions from `log4j-core`...

But the problem is the inverse one: given a library, which project in your org depends on it? Possibly a project tucked away into a repository that a developer maintained that just left last week and that just happens to be deployed on an external facing web server.

It’s easier for Java based apps since you can just scan the machines they’re deployed on, but it’s hard for C and the like.

Yeah, there was also a deployable that ops almost forgot about, but I remembered. But even in C, you can inspect the symbol table of binaries. Unless they got stripped or optimized away, of course.

Edit: yes, deployables that nobody knows about and just sit there for years are a huge problem.

Provided the dependency is dynamically linked, you can find everything running against an old version of the .so after it's been updated by using the 'checkrestart' command. A combination of unattended-upgrades and periodic checkrestart runs makes my life as a debuntu admin much easier.

Note: Yes, I know how many environments this doesn't apply to. But if one person whose environment it does apply to discovers the joy of 'checkrestart' as a result of this comment, it was still worth typing out :D

It's better than the alternative of not sharing code.

Some complex things need to be shared, but something like a library to make directory structures doesn't. It's fine if teams just write their own, or use one from outside the company. At least that way any issues are limited to only impacting that team, and in the long term you end up with much more maintainable apps.

Shared code that isn't treated like an external library(with proper, defined maintainers) is, in my experience, some of the worst technical debt.

> but something like a library to make directory structure

As others comments point out, doing this well is subtle and tricky. This is absolutely the kind of thing that individual teams ought not have to maintain their own copies of.

Agree the best solution would be for this to be baked into the language, or failing that a well-known OSS library.

But if you don't have either, a shared company library is better than the duplication IMO. It does require a company culture that's able to invest in infrastructure, but I think you need that, especially if you're working with languages like C that don't come with as many "batteries included".

The only thing worse than shared code across the company is to not have any shared code across the company.

> All shared code should have a maintainer,

All code should have a maintainer.

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

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.

It’s better to solve these problems than have N copies of common libraries, each with their own subtle bugs, different interfaces, and so on. A very crude approach to solving this is to a) require code review from some core team for changes to common/, b) add lint checks to prevent project a importing code from project b (i.e. non-common code).

Avoiding the problems one can run into with a shared codebase by not sharing at all is the worst solution.

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.

There should be no shared code and all projects should be totally isolated from eachother. If you need shared code, fork it and maintain it to your needs.

> 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.

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.

> 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.)

Ideally they should be turned into open source projects.

Google's https://abseil.io and Facebook's https://github.com/facebook/folly come to mind.

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.

>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.

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?

And semicolons, maybe other things: “myfanciuserinput.mp3 && pwned > /etc/hosts”

That syntax is interpreted by the shell. It means nothing to mkdir() (an OS-level API) or fopen() (a C library call).

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

There's not much context in the story, but I would guess you might want some sanitizing for stuff like ../../../<whatever>/

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.

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().

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.

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.

The C standard does not even have the plain mkdir function. It is defined in POSIX.

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

We are talking about creating directories here...

How would it work on a microcontroller without a filesystem?

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.

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

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.

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.

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.

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.

The moral is the importance of language-theoretic security: 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.

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?

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.

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...

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?

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.

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.

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.

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.

Because most programming languages make it very hard to correctly model data, for example, they lack sum types/discriminated unions.

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.

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.

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.

I wrote a VS code extension and pre-commit hook for this kind of thing:

https://github.com/elanning/checkr

It's a nice inbetween solution for quick stuff not worth writing a full static analysis plugin for.

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.subproces...

- system-like calls with a non-static argument: 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.

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.

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.

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.

Not particularly mainstream, but I remember Ballerina having this:

https://ballerina.io/1.0/learn/by-example/taint-checking.htm...

Taint mode in Perl.

This is my instinct working with Perl. Test the input with a regex and move on.

Better still, with -somebody elses- regexp.

p3rl.org/File::Spec#no_upwards - after calling splitpath and splitdir from the same module - is very much my friend.

(or for advanced mode, Path::Tiny, possibly with the help of App::FatPacker to condense back down to a single script if you're writing something you need to be able to copy around without thinking about dependencies)

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.

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…

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.

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 :)

> 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.

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.

Yes, I’m changing the subject a bit/focusing on an irrelevant (to the story, important otherwise) detail.

> 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.

Reply on news.ycombinator.com