Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix security issues of the argument parser #1133

Merged
merged 3 commits into from
Jul 4, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/tutorial/alphabet/alphabet_gc_content.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ using namespace seqan3;
int main (int argc, char * argv[])
{
std::string input{};
seqan3::argument_parser parser("GC Content", argc, argv);
seqan3::argument_parser parser("GC-Content", argc, argv);
parser.add_positional_option(input, "Specify an input sequence.");
try
{
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/basic_parser_setup.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ using namespace seqan3;

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv}; // initialise myparser
argument_parser myparser{"Game-of-Parsing", argc, argv}; // initialise myparser

// ... add information, options, flags and positional options

Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/disable_version_check.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ using namespace seqan3;

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv, false};
argument_parser myparser{"Game-of-Parsing", argc, argv, false};
// disable version checks permanently --------------------^
}
14 changes: 7 additions & 7 deletions doc/tutorial/argument_parser/small_snippets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,39 +21,39 @@ int main(int argc, char ** argv)
{

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![add_positional_option]
size_t variable{};
parser.add_positional_option(variable, "This is a description.");
//![add_positional_option]
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![add_option]
size_t variable{};
parser.add_option(variable, 'n', "my-number", "This is a description.");
//![add_option]
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![add_flag]
bool variable{};
parser.add_flag(variable, 'f', "my_flag", "This is a description.");
//![add_flag]
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![option_list]
std::vector<std::string> list_variable{};
parser.add_option(list_variable, 'n', "names", "Give me some names.");
//![option_list]
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![positional_option_list]
std::string variable{};
std::vector<std::string> list_variable{};
Expand All @@ -63,15 +63,15 @@ parser.add_positional_option(list_variable, "Give me one or more variables!.");
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![required_option]
std::string required_variable{};
parser.add_option(required_variable, 'n', "name", "I really need a name.", option_spec::REQUIRED);
//![required_option]
}

{
argument_parser parser{"Example Parser", argc, argv};
argument_parser parser{"Example-Parser", argc, argv};
//![input_file_validator]
parser.add_positional_option(args.file_path, "Please provide a tab separated data file.",
input_file_validator{{"tsv"}});
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/solution1.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ void initialize_argument_parser(argument_parser & parser)

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv};
argument_parser myparser{"Game-of-Parsing", argc, argv};

// code from assignment 1
}
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/solution3.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ void initialize_argument_parser(argument_parser & parser, cmd_arguments & args)

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv}; // initialise myparser
argument_parser myparser{"Game-of-Parsing", argc, argv}; // initialise myparser
cmd_arguments args{};

initialize_argument_parser(myparser, args);
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/solution4.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ void initialize_argument_parser(argument_parser & parser, cmd_arguments & args)

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv}; // initialise myparser
argument_parser myparser{"Game-of-Parsing", argc, argv}; // initialise myparser
cmd_arguments args{};

initialize_argument_parser(myparser, args);
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/solution5.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ void initialize_argument_parser(argument_parser & parser, cmd_arguments & args)

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv}; // initialise myparser
argument_parser myparser{"Game-of-Parsing", argc, argv}; // initialise myparser
cmd_arguments args{};

initialize_argument_parser(myparser, args);
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/argument_parser/solution6.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ void initialize_argument_parser(argument_parser & parser, cmd_arguments & args)

int main(int argc, char ** argv)
{
argument_parser myparser{"Game of Parsing", argc, argv}; // initialise myparser
argument_parser myparser{"Game-of-Parsing", argc, argv}; // initialise myparser
cmd_arguments args{};

initialize_argument_parser(myparser, args);
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/concepts/custom_validator_solution2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ static_assert(seqan3::Validator<custom_validator>);
//![main]
int main(int argc, char ** argv)
{
seqan3::argument_parser myparser("Test Parser", argc, argv);
seqan3::argument_parser myparser("Test-Parser", argc, argv);

int32_t variable{};
int16_t variable2{};
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/introduction/introduction_argument_parser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ int main(int argc, char * argv[])
// Create a buffer for the input.
std::string input{};
// Initialise the Argument Parser and add an option.
seqan3::argument_parser parser("My first program", argc, argv);
seqan3::argument_parser parser("My-first-program", argc, argv);
parser.add_positional_option(input, "Give me text.");

// Parse the given arguments and catch possible errors.
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/introduction/introduction_read_fasta.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ int main(int argc, char * argv[])
{
// Receive the filename as program argument.
std::string filename{};
seqan3::argument_parser parser("My first program", argc, argv);
seqan3::argument_parser parser("My-first-program", argc, argv);
parser.add_positional_option(filename, "The filename of the file to read.");
try
{
Expand Down
2 changes: 1 addition & 1 deletion doc/tutorial/ranges/range_solution4.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ using namespace seqan3;

int main(int argc, char ** argv)
{
argument_parser myparser("Vector implementations comparison", argc, argv);
argument_parser myparser("Vector-implementations-comparison", argc, argv);
size_t size{};
bool use_bitvector{};
myparser.add_positional_option(size, "Size of vector");
Expand Down
12 changes: 12 additions & 0 deletions include/seqan3/argument_parser/argument_parser.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include <string>
#include <variant>
#include <vector>
#include <regex>

// #include <seqan3/argument_parser/detail/format_ctd.hpp>
#include <seqan3/argument_parser/detail/format_help.hpp>
Expand Down Expand Up @@ -166,6 +167,11 @@ 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.
rrahn marked this conversation as resolved.
Show resolved Hide resolved
*
* The application name must only contain alpha-numeric characters, '_' or '-',
* i.e. the following regex must evaluate to true: `\"^[a-zA-Z0-9_-]+$\"`.
*
* See the [argument parser tutorial](http://docs.seqan.de/seqan/3.0.0-master-dev/tutorial_argument_parser.html)
* for more information about the version check functionality.
*/
Expand All @@ -175,6 +181,9 @@ class argument_parser
bool version_check = true) :
version_check_dev_decision{version_check}
{
if (!std::regex_match(app_name, app_name_regex))
throw parser_design_error{"The application name must only contain alpha-numeric characters "
"or '_' and '-' (regex: \"^[a-zA-Z0-9_-]+$\")."};
info.app_name = std::move(app_name);
init(argc, argv);
}
Expand Down Expand Up @@ -492,6 +501,9 @@ class argument_parser
//!\brief The future object that keeps track of the detached version check call thread.
std::future<bool> version_check_future;

//!\brief Validates the application name to ensure an escaped server call
std::regex app_name_regex{"^[a-zA-Z0-9_-]+$"};

/*!\brief Initializes the seqan3::argument_parser class on construction.
*
* \param[in] argc The number of command line arguments.
Expand Down
6 changes: 5 additions & 1 deletion include/seqan3/argument_parser/auxiliary.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,11 @@ enum option_spec
*/
struct argument_parser_meta_data // holds all meta information
{
//!\brief The application name that will be displayed on the help page.
/*!\brief The application name that will be displayed on the help page.
*
* The application name must only contain alpha-numeric characters, '_' or '-',
* i.e. the following regex must evaluate to true: `\"^[a-zA-Z0-9_-]+$\"`.
*/
std::string app_name;
//!\brief The version information `MAJOR.MINOR.PATH` (e.g. 3.1.3)
std::string version;
Expand Down
104 changes: 47 additions & 57 deletions include/seqan3/argument_parser/detail/version_check.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ class version_checker
version_checker(std::string name_, std::string const & version_, std::string const & app_url = std::string{}) :
name{std::move(name_)}
{
assert(std::regex_match(name, std::regex{"^[a-zA-Z0-9_-]+$"})); // check on construction of the argument parser

if (!app_url.empty())
{
message_app_update.pop_back(); // remove second newline
Expand All @@ -88,28 +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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

(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?

Copy link
Member

Choose a reason for hiding this comment

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

AH okay, I didn't catch the underscore.

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

👍

}

server_url = std::string{"http://seqan-update.informatik.uni-tuebingen.de/check/SeqAn3_"} +
get_os() + get_bit_sys() + name + "_" + version;

program = get_program();

// build up command for server call
if (!program.empty())
{
std::filesystem::path out_file = cookie_path / (name + ".version");
command = program + " " + out_file.string() + " " + server_url;
#if defined(_WIN32)
command = command + "; exit [int] -not $?}\" > nul 2>&1";
#else
command = command + " > /dev/null 2>&1";
#endif
}
}
//!\}

Expand Down Expand Up @@ -190,12 +176,50 @@ class version_checker

std::cerr << std::flush;

std::string program = get_program();

if (program.empty())
{
prom.set_value(false);
return;
}

// 'cookie_path' is no user input and `name` is escaped on construction of the argument parser.
std::filesystem::path out_file = cookie_path / (name + ".version");

// build up command for server call
std::string command = program + // no user defined input
" " +
out_file.string() +
" " +
std::string{"http://seqan-update.informatik.uni-tuebingen.de/check/SeqAn3_"} +
#ifdef __linux
"Linux" +
#elif __APPLE__
"MacOS" +
#elif defined(_WIN32)
"Windows" +
#elif __FreeBSD__
"FreeBSD" +
#elif __OpenBSD__
"OpenBSD" +
#else
"unknown" +
#endif
#if __x86_64__ || __ppc64__
"_64_" +
#else
"_32_" +
#endif
name + // !user input! escaped on construction of the argument parser
"_" +
version + // !user input! escaped on construction of the version_checker
#if defined(_WIN32)
"; exit [int] -not $?}\" > nul 2>&1"
#else
" > /dev/null 2>&1";
#endif

// launch a separate thread to not defer runtime.
std::thread(call_server, command, std::move(prom)).detach();
}
Expand Down Expand Up @@ -405,18 +429,12 @@ class version_checker
"[APP INFO] :: If you don't wish to receive further notifications, set --version-check OFF.\n\n";
/*Might be extended if a url is given on construction.*/

//!\brief The url to send the server request.
std::string server_url;
//!\brief The application name.
std::string name;
//!\brief The version of the application.
std::string version{"0.0.0"};
//!\brief The regex to verify a valid version string.
std::regex version_regex{"^[[:digit:]]+\\.[[:digit:]]+\\.[[:digit:]]+$"};
//!\brief The program used to perform the server call via a system call (wget, curl, ftp or Invoke-WebRequest).
std::string program;
//!\brief The hard coded system command that executes the `program` with the `server_url`.
std::string command;
//!\brief The path to store timestamp and version files (either ~/.config/seqan or the tmp directory).
std::filesystem::path cookie_path = get_path();
//!\brief The timestamp filename.
Expand All @@ -429,50 +447,22 @@ class version_checker
#if defined(_WIN32)
return "powershell.exe -NoLogo -NonInteractive -Command \"& {Invoke-WebRequest -erroraction 'silentlycontinue' -OutFile";
#else // Unix based platforms.
if (!system("wget --version > /dev/null 2>&1"))
return "wget --timeout=10 --tries=1 -q -O";
else if (!system("curl --version > /dev/null 2>&1"))
return "curl --connect-timeout 10 -o";
if (!system("/usr/bin/env -i wget --version > /dev/null 2>&1"))
return "/usr/bin/env -i wget --timeout=10 --tries=1 -q -O";
else if (!system("/usr/bin/env -i curl --version > /dev/null 2>&1"))
return "/usr/bin/env -i curl --connect-timeout 10 -o";
rrahn marked this conversation as resolved.
Show resolved Hide resolved
// In case neither wget nor curl is available try ftp/fetch if system is OpenBSD/FreeBSD.
// Note, both systems have ftp/fetch command installed by default so we do not guard against it.
#if defined(__OpenBSD__)
return "ftp -w10 -Vo";
return "/usr/bin/env -i ftp -w10 -Vo";
#elseif defined(__FreeBSD__)
return "fetch --timeout=10 -o";
return "/usr/bin/env -i fetch --timeout=10 -o";
#else
return "";
#endif // __OpenBSD__
#endif // defined(_WIN32)
}

//!\brief Returns the name of the operation system (via macros).
static constexpr const char * get_os() noexcept
{
#ifdef __linux
return "Linux";
#elif __APPLE__
return "MacOS";
#elif defined(_WIN32)
return "Windows";
#elif __FreeBSD__
return "FreeBSD";
#elif __OpenBSD__
return "OpenBSD";
#else
return "unknown";
#endif
}

//!\brief Returns either "_64_" or "_32_" depending on the hardware (via macros).
static constexpr const char * get_bit_sys() noexcept
{
#if __x86_64__ || __ppc64__
return "_64_";
#else
return "_32_";
#endif
}

//!\brief Reads the timestamp file if possible and returns the time difference to the current time.
double get_time_diff_to_current(std::string const & str_time) const
{
Expand Down
Loading