Fix security issues of the argument parser #1133

smehringer · 2019-06-25T12:01:01Z

Addresses several things of #1101

Best reviewd commit by commit.

marehr

Thank you for the changes. I have one open question otherwise looks good :)

marehr · 2019-06-25T13:59:04Z

include/seqan3/argument_parser/detail/version_check.hpp

@@ -88,27 +90,12 @@ class version_checker
 #endif
        std::smatch versionMatch;

+        // Ensure version string is not corrupt
        if (!version_.empty() && /*regex allows version prefix instead of exact match */
            std::regex_search(version_, versionMatch, std::regex("^([[:digit:]]+\\.[[:digit:]]+\\.[[:digit:]]+).*")))
        {
            version = versionMatch.str(1); // in case the git revision number is given take only version number


So version = "MALICIOUS CODE"; stays the same, because it does not match the regex and will therefore not be changed?

(note the underscore difference)
if version_ == "some malicious code not matching the regex" then the member version will never be set inside the if clause and remains the default "0.0.0".

Is the "zusammenploebbeln" of the command more clear now with the changes?

AH okay, I didn't catch the underscore.

Is the "zusammenploebbeln" of the command more clear now with the changes?

👍

marehr · 2019-07-01T19:25:36Z

Hmm jenkins and travis are failing.

home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp: In member function ‘virtual void parser_design_error_app_name_validation_Test::TestBody()’:
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:18:138: error: expected primary-expression before ‘parser’
EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:18:138: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:19:138: error: expected primary-expression before ‘parser’
EXPECT_NO_THROW((argument_parser parser{"test-parser1234_foo", 1, argv}));
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:19:138: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:21:189: error: expected primary-expression before ‘parser’
EXPECT_THROW((argument_parser parser{"test parser", 1, argv}), parser_design_error);
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:21:189: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:22:189: error: expected primary-expression before ‘parser’
EXPECT_THROW((argument_parser parser{"test;", 1, argv}), parser_design_error);
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:22:189: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:23:189: error: expected primary-expression before ‘parser’
EXPECT_THROW((argument_parser parser{";", 1, argv}), parser_design_error);
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:23:189: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:24:189: error: expected primary-expression before ‘parser’
EXPECT_THROW((argument_parser parser{"test;bad script:D", 1, argv}), parser_design_error);
^
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:24:189: error: expected ‘)’ before ‘parser’
/home/travis/build/seqan/seqan3/test/unit/argument_parser/argument_parser_design_error_test.cpp:16:18: error: unused variable ‘argv’ [-Werror=unused-variable]
const char * argv[] = {"./argument_parser_test"};
^~~~

marehr · 2019-07-01T19:29:36Z

test/unit/argument_parser/argument_parser_design_error_test.cpp

+{
+    const char * argv[] = {"./argument_parser_test"};
+
+    EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));


I think you should do this:

Suggested change

EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));

EXPECT_NO_THROW((argument_parser{"test_parser", 1, argv}));

Or

Suggested change

EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));

EXPECT_NO_THROW(([]()

{

argument_parser parser{"test_parser", 1, argv};

}()));

codecov · 2019-07-02T08:56:42Z

Codecov Report

Merging #1133 into master will increase coverage by <.01%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   96.42%   96.43%   +<.01%     
==========================================
  Files         196      196              
  Lines        7757     7761       +4     
==========================================
+ Hits         7480     7484       +4     
  Misses        277      277

Impacted Files	Coverage Δ
include/seqan3/argument_parser/auxiliary.hpp	`100% <ø> (ø)`	⬆️
include/seqan3/argument_parser/argument_parser.hpp	`97.39% <100%> (+0.04%)`	⬆️
...de/seqan3/argument_parser/detail/version_check.hpp	`83.09% <88.23%> (+0.24%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4e8fd4...ebfee17. Read the comment docs.

rrahn

one minor docu comment. Otherwise looks good

include/seqan3/argument_parser/argument_parser.hpp

rrahn · 2019-07-02T14:10:39Z

include/seqan3/argument_parser/argument_parser.hpp

@@ -166,6 +167,10 @@ class argument_parser
     * \param[in] argv           The command line arguments to parse.
     * \param[in] version_check  Notify users about app version updates (default true).
     *
+     * \throws seqan3::parser_design_error if the application name contains illegal characters.
+     *
+     * The application name must only contain alpha-numeric characters or '_' and '-' (regex: \"^[a-zA-Z0-9_-]+$\").


Suggested change

* The application name must only contain alpha-numeric characters or '_' and '-' (regex: \"^[a-zA-Z0-9_-]+$\").

* The application name must only contain alpha-numeric characters, '_' or '-', i.e. the following regex must evaluate to true: `\"^[a-zA-Z0-9_-]+$\"`.

rrahn

Little update of the comment 💅

smehringer · 2019-07-03T06:17:55Z

@rrahn Please check the first commit and the last again, I had to rebase after your timeout change was merged.

@h-2 Does /usr/bin/env -i work on FreeBSD/OpenBSD ?

rrahn

minor request.

include/seqan3/argument_parser/detail/version_check.hpp

…he first variable in PATH.

…ty leaks of the version call.

…t and clear.

rrahn

lgtm

smehringer requested a review from marehr June 25, 2019 12:01

marehr reviewed Jun 25, 2019

View reviewed changes

marehr approved these changes Jul 1, 2019

View reviewed changes

marehr reviewed Jul 1, 2019

View reviewed changes

smehringer force-pushed the argument_parser_version_check branch 2 times, most recently from 21d200b to 7fd7ac1 Compare July 2, 2019 08:56

smehringer requested a review from rrahn July 2, 2019 09:47

rrahn requested changes Jul 2, 2019

View reviewed changes

include/seqan3/argument_parser/argument_parser.hpp Show resolved Hide resolved

smehringer force-pushed the argument_parser_version_check branch from 7fd7ac1 to f0c9154 Compare July 2, 2019 13:47

smehringer requested a review from rrahn July 2, 2019 13:47

rrahn reviewed Jul 2, 2019

View reviewed changes

rrahn requested changes Jul 2, 2019

View reviewed changes

smehringer force-pushed the argument_parser_version_check branch 2 times, most recently from ac0915a to 3c37f5a Compare July 3, 2019 06:14

smehringer requested a review from rrahn July 3, 2019 11:32

rrahn requested changes Jul 3, 2019

View reviewed changes

include/seqan3/argument_parser/detail/version_check.hpp Show resolved Hide resolved

smehringer added 3 commits July 4, 2019 08:02

[MISC] Secure program system call by restricting the environment to t…

49742ed

…he first variable in PATH.

[FIX] Restrict the user input of the application name to avoid securi…

be63053

…ty leaks of the version call.

[MISC] Make the construction of the stsyem command string more compac…

ebfee17

…t and clear.

smehringer force-pushed the argument_parser_version_check branch from 3c37f5a to ebfee17 Compare July 4, 2019 06:02

smehringer requested a review from rrahn July 4, 2019 08:17

rrahn approved these changes Jul 4, 2019

View reviewed changes

rrahn merged commit 542b272 into seqan:master Jul 4, 2019

smehringer deleted the argument_parser_version_check branch May 29, 2020 05:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix security issues of the argument parser #1133

Fix security issues of the argument parser #1133

smehringer commented Jun 25, 2019

marehr left a comment

marehr Jun 25, 2019

smehringer Jun 26, 2019

marehr Jul 1, 2019

marehr commented Jul 1, 2019 •

edited

Loading

marehr Jul 1, 2019

marehr Jul 1, 2019 •

edited

Loading

codecov bot commented Jul 2, 2019 •

edited

Loading

rrahn left a comment

rrahn Jul 2, 2019

rrahn left a comment

smehringer commented Jul 3, 2019

rrahn left a comment

rrahn left a comment

	EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));
	EXPECT_NO_THROW((argument_parser{"test_parser", 1, argv}));

-    EXPECT_NO_THROW((argument_parser parser{"test_parser", 1, argv}));
+    EXPECT_NO_THROW(([]()
+    {
+        argument_parser parser{"test_parser", 1, argv};
+    }()));

	* The application name must only contain alpha-numeric characters or '_' and '-' (regex: \"^[a-zA-Z0-9_-]+$\").
	* The application name must only contain alpha-numeric characters, '_' or '-', i.e. the following regex must evaluate to true: `\"^[a-zA-Z0-9_-]+$\"`.

Fix security issues of the argument parser #1133

Fix security issues of the argument parser #1133

Conversation

smehringer commented Jun 25, 2019

marehr left a comment

Choose a reason for hiding this comment

marehr Jun 25, 2019

Choose a reason for hiding this comment

smehringer Jun 26, 2019

Choose a reason for hiding this comment

marehr Jul 1, 2019

Choose a reason for hiding this comment

marehr commented Jul 1, 2019 • edited Loading

marehr Jul 1, 2019

Choose a reason for hiding this comment

marehr Jul 1, 2019 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 2, 2019 • edited Loading

Codecov Report

rrahn left a comment

Choose a reason for hiding this comment

rrahn Jul 2, 2019

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

smehringer commented Jul 3, 2019

rrahn left a comment

Choose a reason for hiding this comment

rrahn left a comment

Choose a reason for hiding this comment

marehr commented Jul 1, 2019 •

edited

Loading

marehr Jul 1, 2019 •

edited

Loading

codecov bot commented Jul 2, 2019 •

edited

Loading