Dollar sign escape

[FrenkenFlores]

IPMI commands get executed in a separate shell where the combination "status" has its own behavior. In our case, Popen starts a new shell, for example by running /bin/bash, and when the BMC password contains the combination above it gets replaced to "bin/bash" (in this case the combination holds the last executed command as a string). I have added a method to Session class that will escape the dollar sign. Such behavior is expected with other signs (like combination "^_") but I didn't find it critical.

[coveralls]

coverage: 69.842% (+0.009%) from 69.833% when pulling a4859d1692f18b04fbf680ee463d5dac284c86fa on FrenkenFlores:dollar_sign_escape into f0df31e096e70842f710bd7294d4ac6ed4511df3 on kontron:master.

[hthiery]

could you please meld the commits into one for a better review. you should also add testcase(s) for issues/enhancements that are not yet covered by a testcase.

thanks

[FrenkenFlores]

I have checked the Codacy Static Code Analysis reports. It has conflict with using hard coded password and Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

[hthiery]

I have checked the Codacy Static Code Analysis reports. It have conflict with using hard coded password and the Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

we can configure codacy to ignore tests, e.g. https://docs.codacy.com/repositories-configure/codacy-configuration-file/#ignore-files

[FrenkenFlores]

I have checked the Codacy Static Code Analysis reports. It have conflict with using hard coded password and the Popen function from the subprocess library. But I wanted to emulate the same behavior as Ipmitool class. Can we ignore the reports and merge it?

we can configure codacy to ignore tests, e.g. https://docs.codacy.com/repositories-configure/codacy-configuration-file/#ignore-files

I can't configure codacy by myself, I will wait for you to skip it.

[FrenkenFlores]

@hthiery Hi!) Is there any update on this PR?

[hthiery]

@hthiery Hi!) Is there any update on this PR?

I just see that calling ipmitool with popen and Shell=True is an issue .

Using e.g.: ./ipmitool.py -vv -I ipmitool -o interface_type=lan -H 10.0.114.137 -U '";$(xterm);"' -P admin bmc info can execute something that is embedded in user or password.

So I think we should change Shell=True to Shell=False

[FrenkenFlores]

Ok. I will look into it)

[FrenkenFlores]

Hi! I have tried to find another way to replicate the same behavior, but I couldn’t. I see that the issue is related to the risk of shell injection. We already have this risk in another part of the library. If we fix it, then the current pull request can be canceled.

    @staticmethod
    def _run_ipmitool(cmd):
        """Legacy call of ipmitool (will be removed in future)."""
        log().debug('Running ipmitool "%s"', cmd)

        child = Popen(cmd, shell=True, stdout=PIPE)
        output = child.communicate()[0]

        log().debug('return with rc=%d, output was:\n%s',
                    child.returncode,
                    output)

        if child.returncode == 127:
            raise RuntimeError('ipmitool command not found')

        return output, child.returncode

[FrenkenFlores]

The main problem is the password—it may include special symbols. If we change the above function _run_ipmitool by setting shell to False and using a list of strings instead of a single string, then the password won’t be altered during execution.

    @staticmethod
    def _run_ipmitool(cmd):
        """Legacy call of ipmitool (will be removed in future)."""
        log().debug('Running ipmitool "%s"', cmd)

        child = Popen(cmd.split(), shell=False, stdout=PIPE)
        output = child.communicate()[0]

        log().debug('return with rc=%d, output was:\n%s',
                    child.returncode,
                    output)

        if child.returncode == 127:
            raise RuntimeError('ipmitool command not found')

        return output, child.returncode