> 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.
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.
> 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 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 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.
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".
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.
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.
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.
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".
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.
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 ")"
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.
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.
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.
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!
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.
> 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.
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.
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.
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?
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?
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.
> 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?
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.
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.
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.
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.
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.
>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.
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
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.
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.
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.
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!
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.
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...
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.
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.
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.
> 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.
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.
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.
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.
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)
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.
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.
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.
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 :)
157 comments
[ 0.29 ms ] story [ 238 ms ] threadLow-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.
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.)
Google's https://abseil.io and Facebook's https://github.com/facebook/folly come to mind.
> 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.
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.
>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.
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.
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.
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.
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…
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.
Edit: yes, deployables that nobody knows about and just sit there for years are a huge problem.
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
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.
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".
All code should have a maintainer.
Avoiding the problems one can run into with a shared codebase by not sharing at all is the worst solution.
Instead you end up in a situation where everyone is responsible for everything, which also means everyone is responsible for nothing.
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.
The error: you should never pass unescaped/untrusted input into a subshell.
In Python, it’s the difference between the safe:
Instead of: 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’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 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.
The snippet breaks for paths starting with a minus, as mkdir will interpret those as another option.
The solution is:
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?
In that example, there would always be a move or copy there, though there are scenarios in which it might be elided.
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.
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.
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.
This is, of course, an extreme example. But there are definitely cases where it matters a lot.
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.
- 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.
[0] https://pkg.go.dev/os#MkdirAll
[1] https://doc.rust-lang.org/std/fs/fn.create_dir_all.html
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?
In C++ that functionality is indeed part of the standard library.
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?
Anyway I just checked and Glib does seem to have it for example.
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?
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?
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.
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.
Your approach makes perfect sense within that context.
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?
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.
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.
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.
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?
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.
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.
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.
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).
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.
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.
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.
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.
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.
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.
Leverage a lookup that says user input “../../../root” == code generated directory path.
You might need some additional checks, like for ../ and symlinks, but you need these regardless whether you execve or write your own mkdirp().
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.
Don't cripple your standard library. If C had a function for this super common need then this problem would never happen.
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.)
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.
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!
Please don't assume people are stupid ;) These sandboxes have been carefully designed and extensively tested.
https://linux.die.net/man/2/openat
The POSIX file system API is a festering sore, an utter disaster. Symlinks are the cause.
https://github.com/elanning/checkr
It's a nice inbetween solution for quick stuff not worth writing a full static analysis plugin for.
- 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.
This is so basic; why do people keep getting it wrong?
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.
Not doing it is just plain incompetence.
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.
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.
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.
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.
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.
This seems like the kind of thing that if done well, could eliminate entire classes of bugs without being too burdensome.
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.
https://ballerina.io/1.0/learn/by-example/taint-checking.htm...
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)
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.
Unit tests shouldn’t be creating directories in the first place - any outside interaction ought to be mocked out anyway.