25 ms·
size_t-to-int vulnerability in Linux’s filesystem layer
- drummer 5y agoC++ engineers check out: Cppcon 2018: Safe numerics by Robert Ramey https://m.youtube.com/watch?v=93Cjg42bGEw https://m.youtube.com/watch?v=93Cjg42bGEw
- Zababa 5y ago> Unfortunately, this size_t is also passed to functions whose size argument is an int (a signed 32-bit integer), not a size_t. Is this the type of things that could be caught by a linter or strict compilation rules? This seems to be to be a failure of the type system.
- gruez 5y agoAFAIK most compilers by default will output a warning in this case.
- jeffbee 5y agoI don't see any warnings for this narrowing parameter conversion. #include "stddef.h" short foo(short a) { return a % 42; } size_t bar(void) { size_t sz = ~0UL; return foo(sz); } https://godbolt.org/z/3ec9v8Pa4 https://godbolt.org/z/3ec9v8Pa4
- xeeeeeeeeeeenu 5y agoIt warns if you add -Wconversion. Unfortunately, that flag generates lots of false positives (at least in gcc), so using it isn't always a good idea.
- jeffbee 5y agoIt isn't ever a good idea. C programmers are just supposed to know about the conversion and promotion rules, every time they add a line, even though the rules are actually insanely complicated. Compiler warnings can't overcome this because programmers have no way of just discovering one of GCC's 726 warning flags, only a few of which are enabled by -Wall and -Wextra, most of which are way too noisy to serve a useful purpose.
- morio 5y agoWhat kind of false positives are you seeing with gcc? Personally I have never seen gcc spitting out a false positive. IMO it's always a good idea to explicitly downcast even if you know that it's 'safe'. That way someone else will see instantly what's going on. The fact that Rust requires it should tell us something.
- xeeeeeeeeeeenu 5y agoFor example, I've just reported this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101537 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101537 You can find more cases in the bugtracker. To be fair, it seems many of them were fixed in recent releases.
- Cloudef 5y agoThe 1 and 0 are ints. It most likely complaints because the sign isn't same.
- ototot 5y agoIn my own small projects, I always add -Wconversion to build configuration. I think the false positive is affordable when you start from small piece of code.
- saurik 5y agoYeah; AFAIK you need something like clang-tidy's cppcoreguidelines-narrowing-conversions check (which everyone should definitely be using). (edit: But I'm wrong! That check is apparently similar to the -Wconversion mentioned by someone else.)
- gruez 5y agoOn MSVC, default project settings I get: warning C4267: 'argument': conversion from 'size_t' to 'short', possible loss of data https://godbolt.org/z/nYeWT7zv6 https://godbolt.org/z/nYeWT7zv6 (/W3 is the default warning level when creating a new project) I saw these warnings so often that I assumed that every compiler had them.
- im3w1l 5y agoToo bad that most projects are so full of integer size and signedness warnings that people get warning fatigue and just completely ignore them.
- loeg 5y agoGCC and Clang (the predominant Linux and Mac compilers) mostly don’t warn by default, you need -Wall or other flags (-Wextra, -Weverything, or specific flags like -Wconversion).
- nly 5y agoThey can't warn by default, because on some platforms long->int isnt a narrowing conversion.
- aaronmdjones 5y agoThe compiler knows what platform it's compiling the code for, though.
- usefulcat 5y agoGCC's -Wconversion has some issues. For example, good luck getting gcc to /not/ emit a warning for this code, in C or C++. I have yet to find the appropriate cast to avoid a warning. Clang does not warn for this. typedef struct { unsigned value : 4; } S; void foo(S* s, unsigned value) { // error: conversion from 'unsigned int' to 'unsigned char:4' may change value s->value = value; } I mean, I guess I can see the rationale.. it's just annoying to have to resort to using pragmas to turn off -Wconversion whenever I need to assign to a bitfield.
- Denvercoder9 5y agoDoes it still warn if you do `s->value = value & 0x0F`? That seems like a reasonable alternative to pragmas if it works.
- ainar-g 5y agoAt this point, you can—and probably should—consider C and “C-with-analysers” two different languages. If you use static (and dynamic, if you have tests) code analysis, making software without these issues is way easier, and doing so without these tools is essentially impossible. Both because of the C language itself as well as because of the culture of “bytes go brrr” and “just use a debugger” that a lot of middle-level C programmers have in my experience.
- kllrnohj 5y agoYes, it is the type of thing caught by a linter or strict compilation rules. https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.html https://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidel... But strict compilation rules (eg, clang's -Weverything) mainly only work if you treat them as errors (so -Werror), and then some of those strict are then also questionable at best, and just outright annoyingly wrong at worst. For example, unused parameter warnings on virtual methods are a waste of time to deal with. It's not a symptom of a bug most of the time, so it being an error just generates workaround churn or you end up just disabling the warning and then maybe that bites you the few times it would have pointed out an actual issue. Beyond the blanket ones like clang's -Weverything, it can otherwise be a job to keep up with compiler upgrades and the vast number of warning options they have.
- heavenlyblue 5y ago> For example, unused parameter warnings on virtual methods are a waste of time to deal with. Why is that even a warning? If at least one of the implementers use a parameter and a warning is shown the warning itself is wrong. That’s just broken implementation of the warning?
- codedokode 5y agoYes, this can be caught by a static analyzer and it is sad that Linux doesn't use it. I wonder, is it because the code quality is low and there would be too many warnings?
- SolarNet 5y agoThey do use them, but only in a narrow fashion. A lot of people have tried to make default kernel wide static analyzers but they are mostly not useful. And it's because the kernel does a lot of non-standard things, mostly because it has to. It is not a normal program.
- gaul 5y agoclang's -Wshorten-64-to-32 can catch this: https://clang.llvm.org/docs/DiagnosticsReference.html#wshorten-64-to-32 https://clang.llvm.org/docs/DiagnosticsReference.html#wshort... The more general-purpose -Wconversion has many false positives, often around int to char conversion. Some functions like int toupper(int) have an unexpected int return type to deal with special out-of-bound values like EOF.
- kvathupo 5y agoCached mirror - https://webcache.googleusercontent.com/search?q=cache:LwH96Xc7ImUJ:https://www.openwall.com/lists/oss-security/2021/07/20/1+&cd=3&hl=en&ct=clnk&gl=us https://webcache.googleusercontent.com/search?q=cache:LwH96X... I'm clueless about security: where does this fall on the scale of non-issue to critical? It strikes me as tending towards the latter, given that it enables unprivileged users to become root. Any insight into past Linux Kernel vulnerabilities that were severe?
- vbezhenar 5y agoLocal root escalation is very common vulnerability. It's almost pointless to expect that attacker will not be able to escalate his privileges given enough time, if he got user account access. One weak layer of defense at most.
- McGlockenshire 5y agoPulling this attack off requires enough access to the machine to either run the unprivileged commands needed to create the exploit condition or to upload a binary/script that runs unprivileged that in turn creates the exploit condition. If the attacker already has that level of unauthorized access, you're already doomed.
- tptacek 5y agoAttacks like this break multitenant computing environments. It's not a threat to your desktop computer or your phone. But it can be a very big deal for hosting environments. It also breaks sandboxing. To whatever extent you're trying to run programs that are somehow jailed, so you can download and run them without worrying about them taking over your system, kernel LPEs break those assurances.
- samus 5y agoAndroid also uses user accounts (one per app) to enforce its security model. Is that what you refer to with sandboxing?
- Deukhoofd 5y agoI see in the mail that Red Hat sent out patches to resolve this. Are those patches already merged, or is this a CVE about a live exploit?
- GrayShade 5y agoStill not fixed in the mainline kernel, it seems.
- cesarb 5y agoThis kind of issue is the reason why some more modern languages like Rust or Go do not have implicit narrowing conversions. For instance, on Rust, trying to simply pass an usize (Rust's equivalent of size_t) to a function which expects an i32 (Rust's equivalent of int) will not compile; the programmer has to write "size as i32" (Rust's equivalent of "(int) size"), which makes it explicit that it might truncate the value at that point. (Some Rust developers argue that even "size as i32" should be avoided, and "size.try_into()" should be used instead, since it forces the programmer to treat an overflow explicitly at runtime, instead of silently wrapping.)
- mnw21cam 5y agoAdd Java to that list.
- csharptwdec19 5y agoSame for C#. Any narrowing truncation needs to be an explicit cast. Widening is typically allowed implicitly, although in the case of the 'decimal' (128 bit struct representing a 'higher precision' floating point) type you still need an explicit cast from a 'double', since there are cases where that conversion can still change the value or fail (i.e. Infinity/NaN)
- pjmlp 5y agoMicrosoft is considering turning on by default checked arithmetic in the 2022 Visual Studio templates, by the way.
- paulddraper 5y agoNoo! This idea is super new and was invented by Go/Rust.
- irogers 5y agoIt can catch people by surprise that Java's narrowing conversions may not preserve the sign. For example, the following is broken: class TimestampedObject implements Comparable<TimestampedObject> { long timestamp; int compareTo(TimestampedObject other) { return (int)(timestamp - other.timestamp); } ... } ErrorProne catches this: https://errorprone.info/bugpattern/BadComparable https://errorprone.info/bugpattern/BadComparable
- mnw21cam 5y agoI love the little nugget in the mitigations section. You can plug the hole for a normal filesystem, but then FUSE filesystems have an additional problem: "if an attacker FUSE-mounts a long directory (longer than 8MB), then systemd exhausts its stack, crashes, and therefore crashes the entire operating system (a kernel panic)." If there's one place other than the kernel where truly defensive programming should be applied, it is systemd.
- CrazyPyroLinux 5y agosystemd? More like systemK! But seriously, are non-systemd systems not vulnerable to the FUSE portion of this? (CVE-2021-33910)
- saurik 5y agoFWIW, I feel like your comment is responding to an implicit critique of systemd, but even if one was warranted I didn't read that comment as implying such (as the premise would just be that systemd is a key place in the stack where you would need to be super careful, not that it is somehow less careful than other projects... even if I might claim as such for at least logging ;P); it could be that I am misinterpreting your comment, though?
- CrazyPyroLinux 5y agoYeah, I think you and a lot of other people misinterpreted my comment, since it was oddly one of my most-downvoted-ever. My "systemK" joke was indeed implying what you said, that systemd is "a key place in the stack where you would need to be super careful." (Almost Kernel-like.) And my question was legitimate, although poorly-researched. Answering myself: CVE-2021-33910 only affects systemd, not all FUSE in general.
- unmole 5y agoWait how does a user space daemon exhausting its stack lead to a kernel panic?
- airocker 5y agoCan we not have one integer type in c that can grow like in js as required and become bigint if too big?
- vbezhenar 5y agoWe can. But it would be very inefficient. There's no point to use C at all, if you can accept that level of performance.
- wyager 5y agoNo, because that would require implicit dynamic allocation, which would defeat the entire point of using C.
- airocker 5y agoOnly when limits are getting breached, otherwise it is one if statement extra on every access. This happens in Java and other languages.
- wyager 5y agoIt doesn’t matter if it only happens occasionally. It’s completely inappropriate for C, which has no implicit memory allocation. There’s not even a clear way to include implicit allocation in the semantics of C.
- gruez 5y ago>that can grow like in js I thought js "integers" are just floating point numbers?
- OnlyLys 5y agoI believe the poster you were replying to was talking about JS's new `BigInt` type. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt https://developer.mozilla.org/en-US/docs/Web/JavaScript/Refe...
- Faaak 5y agoAnyone knows how to try the PoC (https://www.openwall.com/lists/oss-security/2021/07/20/1/1 https://www.openwall.com/lists/oss-security/2021/07/20/1/1) ? For me it crashes into the fork_userns:177 PS: don't need to downvote. Sometimes managers want you to prove that there's a need to patch. It's dumb but it's what it is
- ploxiln 5y agoYour linux distro may already have unprivileged user namespaces disabled. See the "mitigations" section of the post, and check /proc/sys/kernel/unprivileged_userns_clone
- ryandrake 5y agoI've always thought implicit parameter conversion in general (narrowing or otherwise) was fraught at worst, and code smell at best. If some function takes a size_t, why are you passing something other than a size_t to it? If your eventual call into "code you don't control" takes type X, make sure the value that you eventually pass is type X all the way through the code you do control. Even casting is kind of the lazy way out. I used to be pretty dogmatic about this and more than a few times got called a pedantic nitpicker (size_t is just like an int, bro! Don't worry about it--we got to ship in a week!). You can probably find serious bugs in any C or C++ software project simply by turning on the implicit cast warnings.
- _kst_ 5y agoAnd it's not just parameter passing. It can apply to anything that acts like an assignment. That includes assignment (of course), parameter passing, and returning a value from a function.
- greesil 5y agohttps://org.cs.pub.ro/dragos.tarcatu/llvmlinux/commit/058504edd02667eef8fac9be27ab3ea74332e9b4 https://org.cs.pub.ro/dragos.tarcatu/llvmlinux/commit/058504... m->size must be of type size_t. It's slightly mind-blowing to me that casting to a smaller unsigned int can cause a vulnerability. But I guess unintended behavior (not undefined) can do that.
- anyfoo 5y agoIt's very common. All that needs to happen, as just one example, is the variable of the smaller type being used as an index into an array. You might get an out of bounds access, or even just an access to the wrong element.
- anyfoo 5y agoTo add to my own comment, more realistically this happens if the variable is used in the calculation of an index, rather as directly as the index. Though if the array is sufficiently large, and/or the smaller type sufficiently small (short or even char, which are most often 16 or 8 bit nowadays), then at least accessing the wrong element is still common (out of bounds less so). Well, you get the overall idea.
- ddoubleU 5y agoAfaik this should work on Android for some time now too right? Got unrootable old (but still fully working) phone there, might try to play with it.
- jeffbee 5y agoA fun fact is there are 196 instances of `int buflen` in Torvalds' tree today, 95 instances of `int namelen`, and 13 instances of `int pathlen`, also 3000+ `int size`.
- tinus_hn 5y agoWhy does the compiler not warn if you use a 64 bit unsigned integer when a 32 bit signed integer is required?
- technion 5y agoYou actually can pass -Wconversion to a compiler, but it's one of those things that's going to generate a lot of noise and not be a vulnerability 99% of the time. It's not an easy solved problem, because if you annoy developers with noise they stop caring.
- tinus_hn 5y agoIt’s an easily solved problem if you turn on these warnings from the start. It’s not though if your program contains a million of these potential vulnerabilities already. Should we be afraid developers will stop caring? Did they even care in the first place?
- melbourne_mat 5y agoI wonder if we're going to see some more of Torvald's excellent management technique? >PATCH 3.12 108/142] fs/seq_file: fallback to vmalloc allocation >Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> Oh maybe not this time :-)
- habibur 5y ago> deep directory structure whose total path length exceeds 1GB [...] Oh, I didn't know Linux supports GB long path name. On Windows it's limited to something like MAX_PATH_LENGTH which was defined as 200+ chars when I worked on it.
- Already__Taken 5y agoWindows lifted that limit a few years ago
- paavohtl 5y agoAFAIK the limit has been lifted since Windows NT (so since the 90s), but only if you use the obscure NT path prefix (\\?\).
- magicalhippo 5y agoNot that obscure, it's been mentioned on MSDN for every relevant operation since before 2000.
- divingdragon 5y agoIIRC you still need to use a special flag to enable it, right?
- masklinn 5y agoThere are two things necessary for an application to use long paths: * the system must have LongPathsEnabled set (though it might be the default nowadays, not sure) * the application itself must have `{http://schemas.microsoft.com/SMI/2016/WindowsSettings}longPathAware http://schemas.microsoft.com/SMI/2016/WindowsSettings}longPa...` set in its manifest
- dragonwriter 5y ago> Windows lifted that limit a few years ago IIRC, its a mess because it was lifted inconsistently for different access methods (APIs, and as a consequence UI/CLI methods that depend on them), and at least for some in ways which also are or were dependent on how paths are expressed. So it is, or at least has historically been after it was first “lifted”, a minefield of inconsistent, surprising behaviors with plenty of gotchas if you didn’t treat it as if it were still a limit.
- cannabis_sam 5y agoI’m still amazed that we allow compilation of so obviously faulty programs. It’s like you sent a 1kg package through the postal service, and then the recipient gets an envelope containing a piece of cardboard from the original packaging.. And everyone involved is somehow A-OK with all of this. If your programming language silently converts between types (in any direction), just to accommodate the programmer, instead of them specifying what they actually want to compute, you simply have failed as a programming language designer.
- andyjohnson0 5y ago> you simply have failed as a programming language designer That's some hubris. The C language is 49 years old. Dennis Ritchie made reasonable design decisions for the time he found himself in. I think we should be understanding of that, and the network effects that lead to large parts of the world's critical software infrastructure being implemented in C. I don't think he failed at anything. I used to be a C developer. I know how easy it is shoot your foot off in C. I think that, arguably, as an industry we should think twice before building more big and/or critical systems in C. There are better tools now. But we are where we are and it's important to understand how we got here. Castigating our predecessors as failures does them a disservice.
- kaba0 5y agoThough one should mention that most languages can evolve and overcome some of their problems, eg. see PHP which used to be objectively a shitty language and nowadays it is somewhat usable - C underwent basically no improvements in that ridiculous timeframe. So it is not against the original creator but everyone responsible for the language since then.
- cannabis_sam 5y ago>The C language is 49 years old. That’s my point, no shade on K&R though.
- cat199 5y ago
- snvzz 5y ago$ ls -l /boot/vmlinuz-linux -rw-r--r-- 1 root root 9464864 Jul 16 12:59 /boot/vmlinuz-linux That's over 9MB, running in supervisor mode. 2021, and people are still surprised every time a kernel bug with security implications is found. Maybe it is time to look at different OS designs. Large companies such as Google (Fuchsia) or Huawei (HarmonyOS) have begun to pick up on this.
- samus 5y agoMost of it is drivers though, and stuff that is pretty much required to be there unless one's pasttime (or dayjob) is to tailor and harden kernels.
- snvzz 5y agoThere are architectural ways to avoid this problem. Making the kernel as small as possible, and having components and drivers run unprivileged is the primary way to achieve this. Some real examples that do this: Minix3, Haiku, Genode, Fuchsia, Harmony.
- userbinator 5y agoOur exploit requires approximately 5GB of memory and 1M inodes ...so basically 32-bit systems are totally unaffected (and I believe size_t and int are the same size there anyway), but I think bugs like this are easily prevented by simply imposing sane limits --- there is zero reason to even consider allowing a path more than a few K in length, and IMHO even that is overly generous. While I know there's a lot of hate for Windows' traditional 260-char limit, I personally haven't run into it as a developer except by accident (e.g. runaway recursion) and it's very comforting to know that the limit is there so code will fail before it consumes the disk or memory entirely.
- mishac 5y ago> I personally haven't run into it as a developer Clearly not a nodejs developer then! npm's insanely nested dependency graph caused me to hit the 260 character limit relatively regularly. (though this was several years ago, so maybe they have mitigations for that now)
- pfraze 5y agoIIRC this is partly why node_modules moved to a flat structure
- MauranKilom 5y agoWe've had trouble (and had to occasionally shorten module names to something dumb like mdlWthVclsRmvd) because some part of some toolchain would create a path like C:/current/working/directory/that/is/reasonably/deep/testrun/YYYY_MM_DD_hh_mm_ss/code/moduleName/src/build/bin/../../../../../../outputs/moduleName/YYYY_MM_DD_hh_mm_ss/outputFolder/moduleName_testrun_results.txt or some garbage like that (you get the picture). Yes, half the path was taken by descending into some directory that it went out of again straight away. Test runs would fail because of the 260 char limit unless we cut down the module name length. (Thankfully this did not need to be done in the code itself, just in the test run invocation.)
- saagarjha 5y ago> I think bugs like this are easily prevented by simply imposing sane limits In general, no. Attackers are clever and can usually find their way around arbitrary limits when a bug exists. Sometimes such a restriction might stop them, but more often than not they’ll bypass it some other way.
- veltas 5y agoB doesn't have these issues because it only has one integer size, take that C.
- spijdar 5y agoWell, for that matter the original C on PDP-11 wouldn't have either, it also only had one integer size. We should all just blame ISO C ;-)
- verytrivial 5y agoI honestly think all the automatic type promotion and conversion rules of the C family should be officially classified as "cute", namely an example of a childish simplification of a serious issue. I'm a C++ programmer of 20+ years experience and I have never, NEVER, caught myself thinking "gee, I'm glad I don't need to cast this." You ALWAYS think it, you just don't TYPE it, and that is the utterly wrong metric to optimise for. My 2c anyway.
- MauranKilom 5y agoI've had my fair share of annoyance from situations like "I have between one and four hard-coded insertions into this vector, but the compiler yells at me if I try to store the resulting size in an int". Also, tangentially related is the signed/unsigned business which tends to get in the way frequently. For example, OpenMP 2.0 (the only OpenMP version you get to use with MSVC) requires loop indices to be signed, but both std::vector::size and std::vector::operator[] deal with unsigned integers. Casts guaranteed!