6 ms·

What the hell is systemd doing that a 8MB long file path can exhaust its stack? Is it doing some recursive parsing or is it just doing something plain stupid li

by josefx 5y ago

What the hell is systemd doing that a 8MB long file path can exhaust its stack? Is it doing some recursive parsing or is it just doing something plain stupid like using a VLA to store user provided data?

megous 5y ago
Probably unbounded alloca() as always.
- megous 5y ago
  Yep: https://github.com/systemd/systemd/commit/b34a4f0e6729de292cb3b0c03c1d48f246ad896b https://github.com/systemd/systemd/commit/b34a4f0e6729de292c... strdupa(input) without any length check Fix is to replace it with unbounded malloc() instead of checking for sane length first.
  - stjohnswarts 5y ago
    Good find thanks for sharing. And everyone at work gripes about me carrying the size around with a lot of my variables in the form of a struct. It's strictly a reminder to always be checking the size since I'm juggling with shotguns.
    - thayne 5y ago
      The fact that c doesn't have a native concept of an array with length and strings usually use a null byte to determine the end is, IMO c's biggest failing, and it's worst legacy on the wider software world.
      - nextaccountic 5y ago
        This and having any pointer implicitly be nullable
    - GoblinSlayer 5y ago
      Do they even C? It's official ideology: a good programmer compensates shortcomings of the language.
      - stjohnswarts 5y ago
        They seem to be all about the conciseness :) . We have gigabytes of memory, a size parameter isn't going to make a difference haha. The allow me my little idiosyncrasies though so I can't complain.
  - sinsterizme 5y ago
    Shouldn't this PR also use `free` on the duped string before returning? (I never use C so probably missing something but just based on the docs of strdupa...)
    - deleted 5y ago
      [deleted]
    - rtldg 5y ago
      The variable p is now declared with "_cleanup_free_" which is using some compiler cleanup/destructor attribute stuff to run free
      - sinsterizme 5y ago
        ah okay, thank you :)
  - alerighi 5y ago
    This fix to me reduces the performance for nothing. In Linux (or most general on any UNIX system that I saw) a path should not be longer (total) than PATH_MAX, that is typically defined to 4096 bytes. What is the point on allocating something statically at this point? And yes, I know that really that is only a limit of the system call path lenght, and in theory you can work with longer paths (by changing the current directory to a path and then opening a file from there), because filesystems does (stupidly in my opinion) support it. But in reality, how many applications will break? Does it make sense to support them? Also the code in question seems to be dealing with a filename more than a path. A file name shouldn't be longer than NAME_MAX, and that is an hard limit of many (possibly all?) filesystems, as far as I know. So why? It would be simpler and more optimized to just truncate the name at PATH_MAX. Avoid the overflow and the crash but give an error. Why hard limits are considered that bad? We waste time supporting edge cases that no one would really use in a real system (no way someone needs a path longer than 4096 bytes...), for what? In Windows the limit is 260 characters, and nobody seems to be bothered by that, only in Windows 10 you can increase that.
    - wahern 5y ago
      The Linux kernel doesn't have an actual path limit. Nor does Solaris. PATH_MAX is 4096 in glibc and musl libc because setting to it to something like INT_MAX or ULONG_MAX would break a lot of existing code that uses PATH_MAX to size buffers. (Though Solaris does define it as INT_MAX, IIRC.) OTOH, because of the lack of a hard limit there's also code that relies (if accidentally) on paths longer than PATH_MAX.
      - jwilk 5y ago
        Linux does have a limit, at least for some system calls: $ strace -e trace=file perl -e 'open(FH, "<", "/" x 4096)' … openat(AT_FDCWD, "//////////…"..., O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENAMETOOLONG (File name too long) +++ exited with 0 +++
        wahern 5y ago
        I stand corrected. It seems that Linux copies the entire path into a kernel-allocated buffer (see getname and getname_flags in fs/namei.c as called by various syscalls in fs/open.c), rejecting paths longer than PATH_MAX. EDIT: And on Solaris PATH_MAX is 1024 and (AFAICT) Solaris also copies paths into kernel space. It seems I was confusing things with NL_TEXTMAX, which is INT_MAX on glibc (but not Solaris).
  - deleted 5y ago
    [deleted]
- merb 5y ago
  was that a guess? wtf... btw. it would probably be hard to make the same mistake in rust. unless you write your own code for strings or use strdupa, too via libc. I also do never understand why some libraries use "faster" methods everywhere, unless safer ones. it's not like all interfaces to systemd would need to be fast. but they should be secure.
  - megous 5y ago
    Yes. It happened before, so it was not exactly hard to guess. https://capsule8.com/blog/exploiting-systemd-journald-part-1/ https://capsule8.com/blog/exploiting-systemd-journald-part-1...
  - alerighi 5y ago
    It's not a mistake. Allocating that string on the stack it is not a bad idea. Most of the time the string will be short, and thus an allocation on the stack is faster. Consider that in Linux a path is defined to be a maximum length of PATH_MAX, that is defined to 4096 bytes, and a filename (and directory name) shouldn't be longer than FILE_MAX that is 255 bytes. This limits are defined in the headers and I use them always in writing my C programs (if it crashes... you are doing something really wrong!). So how the hell do you have a directory that is more than 8Mb? You shouldn't! The filesystem doesn't support it. It's a matter of the filesystem driver that should reject a path that long in my opinion. Systemd should be fast. It's at the base of the operating system. Also it should consume little memory. You can say, who cares about allocating dynamically a string, or allocating a static buffer of 16Mb, yes we should care, I use Linux computer with 16Mb of RAM, total. Of course they don't run systemd nowadays since it's too big, but in my opinion systemd is good, and I would like to see it more in the embedded world.
    - ehvatum 5y ago
      > The filesystem doesn't support it. Remember that Linux supports hierarchical mounts! You can mount anything at any depth of directory nesting. Even if it were true that MAX_PATH were an FS limitation, you could still nest mounts and encounter absolute paths exceeding MAX_PATH. MAX_PATH is simply the length in bytes of the longest string you should expect system calls to accept as a path parameter. > I use Linux computer with 16Mb of RAM, total. Of course they don't run systemd nowadays since it's too big, but in my opinion systemd is good, and I would like to see it more in the embedded world. It sounds like using systemd is a terrible idea for memory-constrained devices, so you really don’t want to see it in the embedded world.

by josefx 5y ago

megous 5y ago

Probably unbounded alloca() as always.

megous 5y ago
Yep: https://github.com/systemd/systemd/commit/b34a4f0e6729de292cb3b0c03c1d48f246ad896b https://github.com/systemd/systemd/commit/b34a4f0e6729de292c... strdupa(input) without any length check Fix is to replace it with unbounded malloc() instead of checking for sane length first.
- stjohnswarts 5y ago
  Good find thanks for sharing. And everyone at work gripes about me carrying the size around with a lot of my variables in the form of a struct. It's strictly a reminder to always be checking the size since I'm juggling with shotguns.
  - thayne 5y ago
    The fact that c doesn't have a native concept of an array with length and strings usually use a null byte to determine the end is, IMO c's biggest failing, and it's worst legacy on the wider software world.
    - nextaccountic 5y ago
      This and having any pointer implicitly be nullable
  - GoblinSlayer 5y ago
    Do they even C? It's official ideology: a good programmer compensates shortcomings of the language.
    - stjohnswarts 5y ago
      They seem to be all about the conciseness :) . We have gigabytes of memory, a size parameter isn't going to make a difference haha. The allow me my little idiosyncrasies though so I can't complain.
- sinsterizme 5y ago
  Shouldn't this PR also use `free` on the duped string before returning? (I never use C so probably missing something but just based on the docs of strdupa...)
  - deleted 5y ago
    [deleted]
  - rtldg 5y ago
    The variable p is now declared with "_cleanup_free_" which is using some compiler cleanup/destructor attribute stuff to run free
    - sinsterizme 5y ago
      ah okay, thank you :)
- alerighi 5y ago
  This fix to me reduces the performance for nothing. In Linux (or most general on any UNIX system that I saw) a path should not be longer (total) than PATH_MAX, that is typically defined to 4096 bytes. What is the point on allocating something statically at this point? And yes, I know that really that is only a limit of the system call path lenght, and in theory you can work with longer paths (by changing the current directory to a path and then opening a file from there), because filesystems does (stupidly in my opinion) support it. But in reality, how many applications will break? Does it make sense to support them? Also the code in question seems to be dealing with a filename more than a path. A file name shouldn't be longer than NAME_MAX, and that is an hard limit of many (possibly all?) filesystems, as far as I know. So why? It would be simpler and more optimized to just truncate the name at PATH_MAX. Avoid the overflow and the crash but give an error. Why hard limits are considered that bad? We waste time supporting edge cases that no one would really use in a real system (no way someone needs a path longer than 4096 bytes...), for what? In Windows the limit is 260 characters, and nobody seems to be bothered by that, only in Windows 10 you can increase that.
  - wahern 5y ago
    The Linux kernel doesn't have an actual path limit. Nor does Solaris. PATH_MAX is 4096 in glibc and musl libc because setting to it to something like INT_MAX or ULONG_MAX would break a lot of existing code that uses PATH_MAX to size buffers. (Though Solaris does define it as INT_MAX, IIRC.) OTOH, because of the lack of a hard limit there's also code that relies (if accidentally) on paths longer than PATH_MAX.
    - jwilk 5y ago
      Linux does have a limit, at least for some system calls: $ strace -e trace=file perl -e 'open(FH, "<", "/" x 4096)' … openat(AT_FDCWD, "//////////…"..., O_RDONLY|O_LARGEFILE|O_CLOEXEC) = -1 ENAMETOOLONG (File name too long) +++ exited with 0 +++
      - wahern 5y ago
        I stand corrected. It seems that Linux copies the entire path into a kernel-allocated buffer (see getname and getname_flags in fs/namei.c as called by various syscalls in fs/open.c), rejecting paths longer than PATH_MAX. EDIT: And on Solaris PATH_MAX is 1024 and (AFAICT) Solaris also copies paths into kernel space. It seems I was confusing things with NL_TEXTMAX, which is INT_MAX on glibc (but not Solaris).
- deleted 5y ago
  [deleted]
merb 5y ago
was that a guess? wtf... btw. it would probably be hard to make the same mistake in rust. unless you write your own code for strings or use strdupa, too via libc. I also do never understand why some libraries use "faster" methods everywhere, unless safer ones. it's not like all interfaces to systemd would need to be fast. but they should be secure.
- megous 5y ago
  Yes. It happened before, so it was not exactly hard to guess. https://capsule8.com/blog/exploiting-systemd-journald-part-1/ https://capsule8.com/blog/exploiting-systemd-journald-part-1...
- alerighi 5y ago
  It's not a mistake. Allocating that string on the stack it is not a bad idea. Most of the time the string will be short, and thus an allocation on the stack is faster. Consider that in Linux a path is defined to be a maximum length of PATH_MAX, that is defined to 4096 bytes, and a filename (and directory name) shouldn't be longer than FILE_MAX that is 255 bytes. This limits are defined in the headers and I use them always in writing my C programs (if it crashes... you are doing something really wrong!). So how the hell do you have a directory that is more than 8Mb? You shouldn't! The filesystem doesn't support it. It's a matter of the filesystem driver that should reject a path that long in my opinion. Systemd should be fast. It's at the base of the operating system. Also it should consume little memory. You can say, who cares about allocating dynamically a string, or allocating a static buffer of 16Mb, yes we should care, I use Linux computer with 16Mb of RAM, total. Of course they don't run systemd nowadays since it's too big, but in my opinion systemd is good, and I would like to see it more in the embedded world.
  - ehvatum 5y ago
    > The filesystem doesn't support it. Remember that Linux supports hierarchical mounts! You can mount anything at any depth of directory nesting. Even if it were true that MAX_PATH were an FS limitation, you could still nest mounts and encounter absolute paths exceeding MAX_PATH. MAX_PATH is simply the length in bytes of the longest string you should expect system calls to accept as a path parameter. > I use Linux computer with 16Mb of RAM, total. Of course they don't run systemd nowadays since it's too big, but in my opinion systemd is good, and I would like to see it more in the embedded world. It sounds like using systemd is a terrible idea for memory-constrained devices, so you really don’t want to see it in the embedded world.