You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2017/02/10 07:05:08 UTC
Review Request 56527: Disallowed special path components in IDs.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/
-----------------------------------------------------------
Review request for mesos and James Peach.
Bugs: MESOS-7086
https://issues.apache.org/jira/browse/MESOS-7086
Repository: mesos
Description
-------
- Such IDs should lead to surprising or even dangerous agent side
directory structure.
Diffs
-----
src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897
src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2
Diff: https://reviews.apache.org/r/56527/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu
Re: Review Request 56527: Disallowed special path components in IDs.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/#review165142
-----------------------------------------------------------
Ship it!
Ship It!
- James Peach
On Feb. 10, 2017, 7:05 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56527/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 7:05 a.m.)
>
>
> Review request for mesos and James Peach.
>
>
> Bugs: MESOS-7086
> https://issues.apache.org/jira/browse/MESOS-7086
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
>
>
> Diffs
> -----
>
> src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
> src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2
>
> Diff: https://reviews.apache.org/r/56527/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 56527: Disallowed special path components in IDs.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/#review165099
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56526, 56527]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 10, 2017, 7:05 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56527/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 7:05 a.m.)
>
>
> Review request for mesos and James Peach.
>
>
> Bugs: MESOS-7086
> https://issues.apache.org/jira/browse/MESOS-7086
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
>
>
> Diffs
> -----
>
> src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
> src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2
>
> Diff: https://reviews.apache.org/r/56527/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 56527: Disallowed special path components in IDs.
Posted by James Peach <ja...@me.com>.
> On Feb 10, 2017, at 5:02 PM, Jiang Yan Xu <ya...@jxu.me> wrote:
>
>
> This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56527/
>
> On February 10th, 2017, 10:11 a.m. PST, James Peach wrote:
>
> src/common/validation.cpp (Diff revision 1)
> namespace mesos {
> 42
> id == string{os::HOME_DIRECTORY}) {
> '~' is a bad choice for a ID, but by itself it is not a security issue. You ought to check for id[0] != '~' (or just ban it anywhere in the string).
> "~" is "by itself" as much as a security issue as ".." right?
Yep you are right, I forgot that case :)
> but yeah I overlooked other forms of Tilde-Expansion. As jpeach pointed out offline, perhaps instead of disallowing certain charaters, it's easier to only allow certain chars. We should discuss with the community on that though. I'll drop "~" for now.
>
> - Jiang Yan
>
>
> On February 9th, 2017, 11:05 p.m. PST, Jiang Yan Xu wrote:
>
> Review request for mesos and James Peach.
> By Jiang Yan Xu.
> Updated Feb. 9, 2017, 11:05 p.m.
>
> Bugs: MESOS-7086
> Repository: mesos
> Description
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
> Testing
>
> make check.
> Diffs
>
> src/common/validation.cpp (0f1a02286d8431acfee6136e8ada49b0ac746897)
> src/tests/master_validation_tests.cpp (0c2649089d7fd29eb021ac75c71e6a74368577dc)
> src/tests/slave_validation_tests.cpp (3d17799ed04951fb56524db0f5d89347192300b2)
> View Diff
Re: Review Request 56527: Disallowed special path components in IDs.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On Feb. 10, 2017, 10:11 a.m., James Peach wrote:
> > src/common/validation.cpp, line 42
> > <https://reviews.apache.org/r/56527/diff/1/?file=1629239#file1629239line42>
> >
> > '~' is a bad choice for a ID, but by itself it is not a security issue. You ought to check for `id[0] != '~'` (or just ban it anywhere in the string).
"~" is "by itself" as much as a security issue as ".." right? but yeah I overlooked other forms of [Tilde-Expansion](https://www.gnu.org/software/bash/manual/html_node/Tilde-Expansion.html). As jpeach pointed out offline, perhaps instead of disallowing certain charaters, it's easier to only allow certain chars. We should discuss with the community on that though. I'll drop "~" for now.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/#review165141
-----------------------------------------------------------
On Feb. 9, 2017, 11:05 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56527/
> -----------------------------------------------------------
>
> (Updated Feb. 9, 2017, 11:05 p.m.)
>
>
> Review request for mesos and James Peach.
>
>
> Bugs: MESOS-7086
> https://issues.apache.org/jira/browse/MESOS-7086
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
>
>
> Diffs
> -----
>
> src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
> src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2
>
> Diff: https://reviews.apache.org/r/56527/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 56527: Disallowed special path components in IDs.
Posted by James Peach <jp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/#review165141
-----------------------------------------------------------
src/common/validation.cpp (line 36)
<https://reviews.apache.org/r/56527/#comment236952>
Consider "ID must not be empty".
src/common/validation.cpp (line 42)
<https://reviews.apache.org/r/56527/#comment236953>
'~' is a bad choice for a ID, but by itself it is not a security issue. You ought to check for `id[0] != '~'` (or just ban it anywhere in the string).
- James Peach
On Feb. 10, 2017, 7:05 a.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56527/
> -----------------------------------------------------------
>
> (Updated Feb. 10, 2017, 7:05 a.m.)
>
>
> Review request for mesos and James Peach.
>
>
> Bugs: MESOS-7086
> https://issues.apache.org/jira/browse/MESOS-7086
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
>
>
> Diffs
> -----
>
> src/common/validation.cpp 0f1a02286d8431acfee6136e8ada49b0ac746897
> src/tests/master_validation_tests.cpp 0c2649089d7fd29eb021ac75c71e6a74368577dc
> src/tests/slave_validation_tests.cpp 3d17799ed04951fb56524db0f5d89347192300b2
>
> Diff: https://reviews.apache.org/r/56527/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 56527: Disallowed some special path components in
IDs.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/#review166578
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56526, 56527]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 23, 2017, 7:30 p.m., Jiang Yan Xu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56527/
> -----------------------------------------------------------
>
> (Updated Feb. 23, 2017, 7:30 p.m.)
>
>
> Review request for mesos and James Peach.
>
>
> Bugs: MESOS-7086
> https://issues.apache.org/jira/browse/MESOS-7086
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Such IDs should lead to surprising or even dangerous agent side
> directory structure.
>
>
> Diffs
> -----
>
> src/common/validation.cpp 028679d733d1aab65890d3782f18be0ddf3b05e9
> src/tests/master_validation_tests.cpp 53771f6b5492009fe75cbbfc03a2b542856c1070
> src/tests/slave_validation_tests.cpp 588c5b2a6b3528bbf7af8e724bc5813b05678bfb
>
> Diff: https://reviews.apache.org/r/56527/diff/
>
>
> Testing
> -------
>
> make check.
>
>
> Thanks,
>
> Jiang Yan Xu
>
>
Re: Review Request 56527: Disallowed some special path components in
IDs.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56527/
-----------------------------------------------------------
(Updated Feb. 23, 2017, 11:30 a.m.)
Review request for mesos and James Peach.
Changes
-------
Comments. NNFR.
Summary (updated)
-----------------
Disallowed some special path components in IDs.
Bugs: MESOS-7086
https://issues.apache.org/jira/browse/MESOS-7086
Repository: mesos
Description
-------
- Such IDs should lead to surprising or even dangerous agent side
directory structure.
Diffs (updated)
-----
src/common/validation.cpp 028679d733d1aab65890d3782f18be0ddf3b05e9
src/tests/master_validation_tests.cpp 53771f6b5492009fe75cbbfc03a2b542856c1070
src/tests/slave_validation_tests.cpp 588c5b2a6b3528bbf7af8e724bc5813b05678bfb
Diff: https://reviews.apache.org/r/56527/diff/
Testing
-------
make check.
Thanks,
Jiang Yan Xu