1791029 - (CVE-2022-45412) nsLocalFile::GetNativeTarget() may use incorrect string length for Unix symlink target

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Armin Ebert [:arminius]]

When resolving a symlink on a *nix filesystem, the target string may be set to an incorrect length, resulting in a partially uninitialized buffer or truncation.

Testcase

Depending on the build, <img src="file:///proc/self/fd/1"> gives me an error such as

Security Error: Content at http://localhost/ may not load or link to file:///dev/pts/9oc/self/fd/1%00%00%20%00d%00a%00t%00a%00%20%00f%00r%00o%00m%00%20%00l%00o%00c%00a%00l%00h%00o%00s%00t.

or

Security Error: Content at http://localhost/ may not load or link to file:///dev/null%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5%E5.

Details

In nsLocalFileUnix.cpp nsLocalFile::GetNativeTarget() the steps to obtain a symlink's target are roughly:

• Use lstat() on the symlink path.

  struct STAT symStat;
  if (LSTAT(mPath.get(), &symStat) == -1) {
    return NSRESULT_FOR_ERRNO();
  }

• Set target string length to the symlink size given in symStat.st_size.

  int32_t size = (int32_t)symStat.st_size;
  nsAutoCString target;
  if (!target.SetLength(size, mozilla::fallible)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }

• Use readlink() to write the symlink value (without trailing null) to target.

  if (readlink(mPath.get(), target.BeginWriting(), (size_t)size) < 0) {
    return NSRESULT_FOR_ERRNO();
  }

This assumes that the length reported by lstat() equals the number of bytes later written to the buffer by readlink().

However, aside from potential races, this doesn't hold true for some special paths: E.g., on the Linux systems I checked, any of the symlinks in /proc/<pid>/fd/ reports 64 bytes, no matter its target. (Not sure why that is, I haven't found it documented anywhere.) So when resolving a link such as /proc/self/fd/5, target may remain partially uninitialized or truncate the value to 64 bytes.

I suppose it'd make sense here to check the return value of readlink() for the actual number of bytes written.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I don't seem to be able to reproduce this, I don't see any truncation or anything going on locally. But anyways this should be easy to check for.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'm on 5.19.13 fwiw. What's your kernel?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1791029 - Deal with lstat potentially lying in nsLocalFileUnix. r=#xpcom-reviewers

Comment 4

[Armin Ebert [:arminius]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

I don't seem to be able to reproduce this, I don't see any truncation or anything going on locally. But anyways this should be easy to check for.

Could you give the output of ls -l /proc/self/fd or stat /proc/self/fd/0 on the system you've been checking on?

I'm on 5.19.13 fwiw. What's your kernel?

5.19.12 Just upgraded, same.

Comment 5

[Andrew McCreight [:mccr8]]

This seems like it could go really wrong if it happens, but it sounds like it would be difficult to actually exploit because of the conditions required so I'll mark it sec-moderate.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I also see Size: 64... So that's weird, I wonder if I'm somehow I'm just getting lucky with a null in the right place or what not.

File: /proc/self/fd/0 -> /dev/pts/8
  Size: 64        	Blocks: 0          IO Block: 1024   symbolic link
Device: 0,21	Inode: 1658412     Links: 1
Access: (0700/lrwx------)  Uid: ( 1000/  emilio)   Gid: ( 1000/  emilio)
Access: 2022-10-07 00:41:26.669286046 +0200
Modify: 2022-10-07 00:41:26.669286046 +0200
Change: 2022-10-07 00:41:26.669286046 +0200
 Birth: -
# etc

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Deal with lstat potentially lying in nsLocalFileUnix. r=xpcom-reviewers,nika
https://hg.mozilla.org/integration/autoland/rev/0f03945e3b362a0e5df2962c9ec093c5d1e03d1c
https://hg.mozilla.org/mozilla-central/rev/0f03945e3b36

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This was discussed previously in bug 1665863 but it didn't seem high-priority at the time; that may have been a mistake.

It looks like this patch will crash if the actual symlink target is longer than the buffer. I think the previous behavior in that case would have been silently truncating it, so arguably this is an improvement, but it's not great. Edit: no, I was confusing readlink with the printf family there; it will still silently truncate the path.

The way this normally works is to check the return value of readlink and increase the buffer size as needed:

$ strace -fereadlink,readlinkat readlink /proc/self/fd/4
readlink("/proc/self/fd/4", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 64) = 64
readlink("/proc/self/fd/4", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 128) = 72
…
$ strace -fereadlink,readlinkat ls -l /proc/self/fd/4
readlink("/proc/self/fd/4", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 65) = 65
readlink("/proc/self/fd/4", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 130) = 72
…

And some tools don't even try lstat in the first place:

$ strace -fereadlink,readlinkat readlink /tmp/l
readlink("/tmp/l", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 64) = 64
readlink("/tmp/l", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 128) = 72
…
$ strace -fereadlink,readlinkat ls -l /tmp/l
readlink("/tmp/l", "/tmp/aaaaaaaaaaaaaaaaaaaaaaaaaaa"..., 73) = 72
…

Note that ls's first attempt is the expected size plus one, to distinguish the expected result from overflow.

Comment 9

[Donal Meehan [:dmeehan]]

:emilio this grafts cleanly to esr102, if you could submit an ESR uplift request when ready

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9297483 [details]
Bug 1791029 - Deal with lstat potentially lying in nsLocalFileUnix. r=#xpcom-reviewers

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Avoids uninit memory on some cases.
• User impact if declined: see comment 0
• Fix Landed on Version: 107
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively straight-forward fix. It could be improved (see jld's last comment), but should be a straight improvement and prevent the uninit memory use.

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9297483 [details]
Bug 1791029 - Deal with lstat potentially lying in nsLocalFileUnix. r=#xpcom-reviewers

Approved for 102.5esr.

Comment 12

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/084a54264928

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt