When piped into, nsd-checkzone cannot open /dev/stdin

[dcepelik]

When piped into, nsd-checkzone cannot open /dev/stdin:

130 ~% echo foobar | nsd-checkzone foo /dev/stdin
[2024-09-05 19:50:06.195] nsd-checkzone[828]: error: Cannot open /dev/stdin, no such file
zone foo file /dev/stdin has 1 errors

But invocation with /dev/stdin works when executed without being piped into. The difference in the invocations is:

# /dev/stdin always points to /proc/self/fd/0
~% ls -l /dev/stdin
lrwxrwxrwx 1 root root 15 Aug 26 15:01 /dev/stdin -> /proc/self/fd/0

# Points to pts when invoked from the shell
~% ls -l /proc/self/fd/0
lrwx------ 1 d d 64 Sep  5 19:53 /proc/self/fd/0 -> /dev/pts/11

# Points to a pipe when piped into
~% echo foobar | ls -l /proc/self/fd/0
lr-x------ 1 d d 64 Sep  5 19:53 /proc/self/fd/0 -> 'pipe:[3045139]'

This used to work, so something must have changed recently. I tracked this down to simdzone/src/zone.c (in the Git submodule) in resolve_path (probably).

https://github.com/NLnetLabs/simdzone/blob/7efedd672462d1062d5a73fb9c033ce75e8b2c14/src/zone.c#L149-L174

The code seems a bit fishy to me; at the very least the following is bound to produce confusing diagnostics:

https://github.com/NLnetLabs/simdzone/blob/7efedd672462d1062d5a73fb9c033ce75e8b2c14/src/zone.c#L173

Overall, the whole file-reading logic seems overly complex. Couldn't we just fopen the argument (and check errno) and be done with it? I may be wrong, I only read the code superficially and of course don't know the whole story. But this shouldn't be too hard I think?

I'm running nsd-checkzone 4.10.0. Please let me know if I can provide further info or help with debugging and/or testing. :)

Debugged with @trueMiskin while investigating a failing DNSSEC integration.

[k0ekk0ek]

Hi @dcepelik. Thanks for reporting! (And sorry for taking so long to respond.)

With 4.10.0 we started using simdzone to improve zone loading performance.

We use the resolve_path to ensure we have the full path with symlinks resolved to detect cyclic includes. If the file resolves to pipe:[xxx] opening the actual path probably fails because it's a special "file" (device)? We allow reading from stdin by specifying - since 4.10.1. That's something you can use as an alternative for the time being.

There's nothing really "fishy" going on if you ask me(?) We simply prepended the basedir of the includer if the path was relative and then resolved it. Since 4.10.1 we prepend the current working directory. We can probably catch this case by testing if the file is a device, possibly even detecting if the resolved path starts with a / and using the original path if it doesn't?

[dcepelik]

Hi @dcepelik. Thanks for reporting! (And sorry for taking so long to respond.)

Hi @k0ekk0ek, no problem!

We use the resolve_path to ensure we have the full path with symlinks resolved to detect cyclic includes. If the file resolves to pipe:[xxx] opening the actual path probably fails because it's a special "file" (device)?

We allow reading from stdin by specifying - since 4.10.1. That's something you can use as an alternative for the time being.

Ah, right; I'm on 4.10.0. I'll update then :)

There's nothing really "fishy" going on if you ask me(?)

"Fishy" certainly was a poor choice of words, sorry. I was surprised by the amount of ad-hoc string manipulation. In my experience, that's where the nasty bugs happen, so I tend to avoid it if possible.

We simply prepended the basedir of the includer if the path was relative and then resolved it.

I looked at the code a bit more. The resolve_path function is static and only used in one place in open_file. There, it's always resolved relative to the current working directory (if relative). Unless I'm mistaken, you paste includer (which is always the current working directory) and include; then hand this over to realpath to resolve?

In that case, the whole thing simplifies to

char *abs = realpath(include, NULL);
if (!abs)
        return ...; // errno contains the error

(And that's probably just as portable as the current solution, without having to special-case for Win32.)

Since 4.10.1 we prepend the current working directory.

Right! So perhaps the original logic was necessary, but not anymore? This could make for a very nice clean-up.

Also, nitpick:

int length = snprintf(
  buffer, sizeof(buffer), "%s/%s", includer, include);
if (length < 0)
  return ZONE_OUT_OF_MEMORY;

This doesn't seem right to me: how do we know the cause is insufficient memory? Also, there's no check for length >= sizeof(buffer), and I don't immediately see why that couldn't cause problems down the road (for example, result in including other file than desired).

[dcepelik]

To resolve the original issue:

• Would it make sense to pass the original file name to fopen (since that will resolve relative paths relative to current working directory anyway) and use realpath only for the purpose of cycle detection? (Mind the race though.)

• Would it make sense to instead detect include cycles/loops by limiting include depth? That's foolproof and probably good enough in practice. (That's what I would probably do.)

Would you please point me to the cycle-detection logic? I wanted to check so that I don't suggest nonsense but couldn't find it.

If the file resolves to pipe:[xxx] opening the actual path probably fails because it's a special "file" (device)?

From proc(5):

For file descriptors for pipes and sockets, the entries will be
symbolic links whose content is the file type with the inode.  A
readlink(2) call on this file returns a string in the format:

    type:[inode]

For example, socket:[2248868] will be a socket and its inode is
2248868.  For sockets, that inode can be used to find more
          information in one of the files under /proc/net/.

In other words, what realpath returns is a special-format entry and not an absolute file path; the original name (/dev/stdin) refers to something (an anonymous pipe) that has no file name to begin with.

We can probably catch this case by testing if the file is a device

So, it's not a device file, it's a symlink with special content.

[k0ekk0ek]

The include checks are done here. It does both, cyclic check and include depth. I prefer to leave that detection as-is.

I think we indeed just use the name in the the include directive for fopen 👍 (we should still see if resolve_path succeeds though, so probably best to place a comment above the fopen call). And if we do that, then yes, we may as well simplify the call to realpath. Same is probably true for the Windows code, though maybe we should not mess with it too much(?) We can remove includer from the signature to resolve_path though and call _getcwd only for Windows.

We don't need to worry about the race. I mean yes, theoretically one can update the link etc, but at that point updating the contents of an actual file is an attack vector too(?)

"Fishy" certainly was a poor choice of words, sorry.
No worries.

Do you want to make the changes and issue a PR or do you prefer I do so?

[dcepelik]

The include checks are done here. It does both, cyclic check and include depth. I prefer to leave that detection as-is.

Ah! Of course. I forgot to look into the submodule. Thank you :)

I think we indeed just use the name in the the include directive for fopen 👍 (we should still see if resolve_path succeeds though, so probably best to place a comment above the fopen call). And if we do that, then yes, we may as well simplify the call to realpath.

OK

Same is probably true for the Windows code, though maybe we should not mess with it too much(?) We can remove includer from the signature to resolve_path though and call _getcwd only for Windows.

I'm happy to fix both as long as somebody else is able to test the changes on Windows.

We don't need to worry about the race. I mean yes, theoretically one can update the link etc, but at that point updating the contents of an actual file is an attack vector too(?)

I assumed the race might happen if another program is re-generating zones and the reload of nsd happens at an unfortunate moment. Then it could fool the cycle detection code, leading to a false positive or a false negative.

I think I understand now: there's a depth limit to make sure you don't get stuck in an infinite loop, which is simple and foolproof but probably not too user-friendly (what caused the loop?) and this realpath-based mechanism which is able to produce meaningful diagnostics, correct?

If that's the case, false negatives are not possible. I think that's good enough.

Do you want to make the changes and issue a PR or do you prefer I do so?

I would be happy to provide a fix.

[k0ekk0ek]

We have Windows tests in place and I can do some manual testing in a VM.