Skip to content

filesystem: path sanity checking for fileReadToEnd().#5765

Merged
htuch merged 3 commits intoenvoyproxy:masterfrom
gsserge:filereadtoend
Feb 5, 2019
Merged

filesystem: path sanity checking for fileReadToEnd().#5765
htuch merged 3 commits intoenvoyproxy:masterfrom
gsserge:filereadtoend

Conversation

@gsserge
Copy link
Contributor

@gsserge gsserge commented Jan 30, 2019

Do not allow Filesystem::fileReadToEnd() for files in
/dev, /proc and /sys.

Risk Level: Low
Testing: New unit test.
Fixes: #3170

Signed-off-by: Sergey Glushchenko gsserge@gmail.com

Do not allow Filesystem::fileReadToEnd() for files in
/dev, /proc and /sys.

Risk Level: Low
Testing: New unit test.
Fixes: #3170

Signed-off-by: Sergey Glushchenko <gsserge@gmail.com>
@gsserge gsserge mentioned this pull request Jan 30, 2019
Copy link
Member

@venilnoronha venilnoronha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

}

TEST_F(FileSystemImplTest, fileReadToEndBlacklisted) {
EXPECT_THROW(Filesystem::fileReadToEnd("/dev/urandom"), EnvoyException);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for /proc and /sys?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

Signed-off-by: Sergey Glushchenko <gsserge@gmail.com>
@gsserge
Copy link
Contributor Author

gsserge commented Jan 30, 2019

Updated PR based on the feedback.

Copy link
Member

@venilnoronha venilnoronha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be good to add the same check across other file system operations as well.

@gsserge
Copy link
Contributor Author

gsserge commented Jan 30, 2019

I think it'd be good to add the same check across other file system operations as well.

I mentioned that in in the issue #3170 . Would you prefer that as part of this PR, or in the follow-up?

@htuch htuch self-assigned this Jan 30, 2019
htuch
htuch previously approved these changes Feb 5, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, sorry I missed his one earlier, can you merge master?

Signed-off-by: Sergey Glushchenko <gsserge@gmail.com>
@htuch htuch merged commit f23d808 into envoyproxy:master Feb 5, 2019
@gsserge gsserge deleted the filereadtoend branch February 5, 2019 20:45
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
Do not allow Filesystem::fileReadToEnd() for files in
/dev, /proc and /sys.

Risk Level: Low
Testing: New unit test.
Fixes: envoyproxy#3170

Signed-off-by: Sergey Glushchenko <gsserge@gmail.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants