OpenSCAP / openscap

NIST Certified SCAP 1.2 toolkit
https://www.open-scap.org/tools/openscap-base
GNU Lesser General Public License v2.1
1.34k stars 370 forks source link

"Unable to open file: ... [oscap_source.c:285]" When profile is left blank. #1696

Open BloodBlight opened 3 years ago

BloodBlight commented 3 years ago

Description of Problem:

When we were scripting a report, I left a variable blank by accident. This causes an error error when [--profile] is left blank that is not related to that input.

This sent me down a rabbit hole (as we have very strict volume mount permissions), but it was just a PBKAC error.

Sample execution:

CPEDictionary="/usr/share/xml/scap/ssg/content/ssg-rhel7-cpe-dictionary.xml"
XCCDF="/usr/share/xml/scap/ssg/content/ssg-rhel7-xccdf.xml"
oscap xccdf eval --profile $profile --results /tmp/temp.xml --report /tmp/temp.html --cpe $CPEDictionary $XCCDF

Results in: OpenSCAP Error: Unable to open file: '/reports/temp.xml' [oscap_source.c:285]

This is a small bug (as the error returned is not related to the problem), but maybe add some input verification on this?

THANKS!!!

OpenSCAP Version:

All up to 1.3.3

Operating System & Version:

RHEL 7

jan-cerny commented 3 years ago

I think it's only a problem in command line arguments parser. When $profile is an empty variable it thinks that --results is the ID of the profile and then it takes /tmp/temp.xml as the name of the XCCDF file and ignores the rest.

evgenyz commented 3 years ago

I think it's only a problem in command line arguments parser. When $profile is an empty variable it thinks that --results is the ID of the profile and then it takes /tmp/temp.xml as the name of the XCCDF file and ignores the rest.

I'm quite lost here. What is the proposed 'fix'? Don't allow to use profiles with names starting with a double (or a single) dash?

I'd close it with not a bug.

jan-cerny commented 3 years ago

@evgenyz It can be to improve the command line arguments parsing. If something like that happened with a Python program using argparse, it would print out: oscap: error: argument --profile: expected one argument

evgenyz commented 3 years ago

@evgenyz It can be to improve the command line arguments parsing. If something like that happened with a Python program using argparse, it would print out: oscap: error: argument --profile: expected one argument

Are you sure? Would it mean that this Python program won't allow me to use profile named --results?

jan-cerny commented 3 years ago

It won't allow you. You will have to use '\-\-results'.

evgenyz commented 3 years ago

It won't allow you. You will have to use '\-\-results'.

Well, if you feel it like an improvement... On the other hand - we won't update 1.2 branch.

jan-cerny commented 3 years ago

this issue is reproducible also on 1.3.3

evgenyz commented 3 years ago

this issue is reproducible also on 1.3.3

Okay. Would it break API in your opinion? I mean, the fix.

BloodBlight commented 3 years ago

It won't allow you. You will have to use '\-\-results'.

Well, if you feel it like an improvement... On the other hand - we won't update 1.2 branch.

I would have to agree with @jan-cenry, I would consider that to be the proper way of handling your example (though I probably would allow that name).

It might be worth seeing if there is already a POSIX compliant argument processor that you could just import?

POSIX Standard: https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html

It might bring some pain to change things, but the way it is now is possibly open to abuse (edge cases, but).

evgenyz commented 3 years ago

I would have to agree with @jan-cenry, I would consider that to be the proper way of handling your example (though I probably would allow that name).

You have to forbid that name or else this case would be indistinguishable from "proper" invocation. Honestly, I don't see any problem with this command:

CPEDictionary="/usr/share/xml/scap/ssg/content/ssg-rhel7-cpe-dictionary.xml"
XCCDF="/usr/share/xml/scap/ssg/content/ssg-rhel7-xccdf.xml"
oscap xccdf eval --profile --results /tmp/temp.xml --report /tmp/temp.html --cpe $CPEDictionary $XCCDF

You would get what you asked for...

BloodBlight commented 3 years ago

"--results" should terminate the previous option if you want to be POSIX compliant.

An edge case would be someone abusing an automated system to feed other command into your application. Say I entered into a GUI that didn't have proper escaping: ..--profile $MyVar

And a nefarious or unknowing user entered: "Check for; rm -rf /"

Very much an edge case and is not completely dependent on this code, but could be seen as allowable under the applications guidelines.

Switching it to requiring quotes would result in:

..--profile "$MyVar"

So: ..--profile="Check for; rm -rf /"

This would be compliant and I think you would only need to seek and replace quotes in any garbage input filter.

Just my 2 cents though.