bootstrap: allow /dev/fd/<fd> paths for config files #7279

banks · 2019-06-14T15:50:20Z

Signed-off-by: Paul Banks banks@banksco.de

Description:

As noted in #7528 the newly added sanity checking of possibly sensitive file paths prevents legitimate usage of passing bootstrap via a non-CLOEXEC file descriptor from a generator helper that execs Envoy.

This PR relaxes the validation such that any path resolving to a canonical path with the prefix /dev/fd/ is considered valid.

Risk Level: Low
Testing: Unit test case is added that was failing before the change and passes afterwards. In addition I've manually verified that the old behaviour of allowing /dev/fd/ paths works with my dev binary.
Docs Changes: None, I didn't find this limitation documented currently.
Release Notes: N/A (I didn't see the original change to make /dev invalid in 1.10 notes so I assume this doesn't warrant going in either.)

Fixes #7258

banks · 2019-06-14T16:11:45Z

Hmm the test failure seems legit - at least filesystem_impl_test is failing in CI. Passed on my machine. I'll take a look.

lizan · 2019-06-14T21:21:20Z

@banks I guess it is due to on some system {/dev/fd,/0} is a symlink, so the canonical path is outside of it. on my Linux /dev/fd/0 points to /dev/pts/4 (where it links to depends on the process I think.

banks · 2019-06-15T07:06:50Z

Thanks, I think you’re right. I guess that makes this not the simple fix I hoped. I’ll take another look next week but might need to abandon this if the list of possible places is large across different systems.

…

On 14 Jun 2019, at 22:21, Lizan Zhou ***@***.***> wrote: @banks I guess it is due to on some system {/dev/fd,/0} is a symlink, so the canonical path is outside of it. on my Linux /dev/fd/0 points to /dev/pts/4 (where it links to depends on the process I think. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

banks · 2019-06-17T12:09:25Z

source/common/filesystem/posix/filesystem_impl.cc

+  // /dev/fd/* _before_ resolving the canonical path.
+  if (absl::StartsWith(path, "/dev/fd/")) {
+    return false;
+  }


This seems to be the simplest approach here rather than trying to research all the different OS implementations of this path and white list every possible ending point.

I'm not sure if there are any specific downsides of bypassing realpath here?

Sorry for the delay, been catching up on backlog after OOO. I think this is fine for now, but can you leave a TODO to switch to https://en.cppreference.com/w/cpp/filesystem/absolute when we have C++17? The main downside of what you have is that there is no relative path normalization, which we want independent of symlink following.
/wait

Added the TODO. I hope this is appropriate - I added it under your username as it seems to be more likely you will have context for this later than I will!

banks · 2019-06-17T13:31:04Z

/retest

repokitteh-read-only · 2019-06-17T13:31:11Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7279 (comment) was created by @banks.

see: more, trace.

banks · 2019-06-17T13:31:22Z

Coverage test fail downloading some dependency...

stale · 2019-06-24T14:29:39Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

banks · 2019-06-24T16:20:18Z

@htuch since you were assigned on the issue, would this be of interest to merge? Thanks!

stale · 2019-07-06T09:14:41Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

banks · 2019-07-07T18:26:42Z

Hey @htutch, is this TODO what you meant? Thanks!

…

On 6 Jul 2019, at 11:15, stale[bot] ***@***.***> wrote: This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

htuch

Thanks!

htuch · 2019-07-09T17:28:12Z

@banks yep. Can you squash, fix DCO and force push? I can merge when CI DCO check passes.

Signed-off-by: Paul Banks <banks@banksco.de>

banks · 2019-07-11T08:06:49Z

@htuch sorry, been travelling this week I think this should all be good to go now once CI completes.

htuch

Thanks!

mattklein123 assigned htuch Jun 14, 2019

banks commented Jun 17, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 24, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 24, 2019

repokitteh-read-only bot added waiting and removed waiting labels Jun 26, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 6, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 7, 2019

htuch previously approved these changes Jul 9, 2019

View reviewed changes

htuch added the waiting label Jul 9, 2019

Allow /dev/fd/<fd> paths for config files. Fixes #7258

4547db8

Signed-off-by: Paul Banks <banks@banksco.de>

banks dismissed htuch’s stale review via 4547db8 July 11, 2019 08:05

repokitteh-read-only bot removed the waiting label Jul 11, 2019

htuch approved these changes Jul 11, 2019

View reviewed changes

htuch merged commit fcec70f into envoyproxy:master Jul 12, 2019

banks deleted the allow-dev-fd-config branch July 12, 2019 13:26

bootstrap: allow /dev/fd/<fd> paths for config files #7279

bootstrap: allow /dev/fd/<fd> paths for config files #7279

Uh oh!

Conversation

banks commented Jun 14, 2019

Uh oh!

banks commented Jun 14, 2019

Uh oh!

lizan commented Jun 14, 2019

Uh oh!

banks commented Jun 15, 2019 via email

Uh oh!

banks Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

htuch Jun 26, 2019

Choose a reason for hiding this comment

Uh oh!

banks Jun 29, 2019

Choose a reason for hiding this comment

Uh oh!

banks commented Jun 17, 2019

Uh oh!

repokitteh-read-only bot commented Jun 17, 2019

Uh oh!

banks commented Jun 17, 2019

Uh oh!

stale bot commented Jun 24, 2019

Uh oh!

banks commented Jun 24, 2019

Uh oh!

stale bot commented Jul 6, 2019

Uh oh!

banks commented Jul 7, 2019 via email

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Jul 9, 2019

Uh oh!

banks commented Jul 11, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!