Possible Log Injection

[ZuhairORZaki]

In file: attach.py, there is a method that stores a json field value into logs without validating the data. This allows an attacker to corrupt the log file structure.

#attach.py, Line: 213
log.debug(\"Attached a subscription for {name}\".format(name=pool_json[\"productName\"])                         		                            )

The pool_json json object comes from pool field of ent json object.

#attach.py, Line: 206
for ent in ents:
    pool_json = ent[\"pool\"]

Here ents comes from invoking attach_pool method of attach_service.

#attach.py, Line: 206
ents = attach_service.attach_pool(pool, self.options.quantity)

The attach_pool method down the line makes a POST request to a server. In the case certificate verification is not required, its possible for an attacker to send a forged response in which productName field contains special characters i.e. \n. This can disrupt the log structure.

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

[ptoscano]

Hi @ZuhairORZaki,

In the case certificate verification is not required, its possible for an attacker to ...

... to a lot of bad things, actually, since subscription-manager relies on the content that is sent by the remote (Candlepin) server. An example of this are the serial numbers for the entitlement certificates: the serials are used as filename when writing the certificates, e.g. /etc/pki/entitlement/<SERIAL>.pem. I'm sure we can find lots more of these cases of "assume that the content is not malicious" around the subscription-manager codebase.

Hence, if an user chooses to ignore the SSL certificate validation, it is already risking potentially everything, so I do not see how this small detail would make the situation worse. Note also that the default log level is INFO, so the mentioned log.debug() call would not be in the log file by default unless the user/sysadmin explicitly sets the log level to DEBUG. (Setting to DEBUG is done to, well, debug issues in subscription-manager, and nothing you would keep enabled by default otherwise.) As you see, it's another explicit decision here.

To sum up: while I can see the minor issue here, and fixing this would be sort of straightforward (we have a small helper for this, terminal_printable_content()), I honestly do not see how this would make any positive effect. The scenario in which this would help (i.e. connecting to untrusted servers) already has way more potential "problems" than this.

What is the actual issue that triggered this report? Anything public we can take a look?

[ZuhairORZaki]

Sorry for the late reply @ptoscano

As mentioned in our report, we are doing work with the Alpha Omega project to run static analysis tools and report the results. Our tool Intelligent Code Repair (iCR) came up with this because it was looking for injection opportunities in the code. There were no runtime observations.

We understood and pointed out in the bug report that there are certain assumptions that need to be true to make this a serious issue.As you pointed out, if a user chooses to ignore the SSL at least other errors happen. But at least we are assured from this finding that the only injection problem in this case will be the log injection that was found out.

Since you suggested that the fix is easy, please consider doing the security hardening anyway.

[ptoscano]

Our tool Intelligent Code Repair (iCR) came up with this
[...]
But at least we are assured from this finding that the only injection problem in this case will be the log injection that was found out.

As I asked, unless I see what are the results of this scanning, this is not a conclusion that I, as one of the maintainers of subscription-manager, can draw myself.

[ZuhairORZaki]

Sorry for misunderstanding

Here is the actual scan result reported by Intelligent Code Repair (iCR)

In file: attach.py, there is a method that stores user-provided data into logs without validating the data. This allows an attacker to corrupt the log file structure. iCR suggested proper sanitization before user-provided data can be added to logs.

--- src/subscription_manager/cli_command/attach.py
+++ src/subscription_manager/cli_command/attach.py
@@ -210,6 +210,22 @@
                                     name=pool_json["productName"]
                                 )
                             )
+                            '''
+                            ***************** OpenRefactory Warning *****************
+                            Possible Log injection!
+                            Path:
+                            	File: attach.py, Line: 204
+                            		ents = attach_service.attach_pool(pool, self.options.quantity)
+                            		Variable ents is assigned a tainted value.
+                            	File: attach.py, Line: 207
+                            		pool_json = ent["pool"]
+                            		Variable pool_json is assigned a tainted value.
+                            	File: attach.py, Line: 213
+                            		log.debug(
+                            		                                "Attached a subscription for {name}".format(name=pool_json["productName"])
+                            		                            )
+                            		Tainted information is passed through a method invocation and is used in a sink.
+                            '''
                             log.debug(
                                 "Attached a subscription for {name}".format(name=pool_json["productName"])
                             )

Please let me know if this is sufficient enough