Merge ~raharper/curtin:fix/udevadm-info-shlex-quote into curtin:master

[Ryan Harper]

udev: use shlex.quote when shlex.split errors on shell-escape chars

The udev database may include shell escape characters in the output.
We want to avoid using shlex_quote unless needed as curtin does not
use the quoted value in our code and applying it to all of the values
breaks parsing of the udevadm info content.

If we encounter a ValueError while invoking shlex.split() then we
first use shlex_quote() and try parsing again and if that fails we
will try replacing shell-quote values with '_'. We log warning
whenever we have to quote or replace values.

- Python2.7 does not have shlex.quote, use pipes.quote (which was
  renamed to shlex.quote in Py3:
  https://docs.python.org/2/library/pipes.html#pipes.quote)
- Add unittest with original info dump from bug submitters system

LP: #1875085

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:17f47538f122e8be3443d7b6cf1ad70744a96998
https://jenkins.ubuntu.com/server/job/curtin-ci/66/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/66/
    FAILURE: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/66/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/66//rebuild

review: Needs Fixing (continuous-integration)

[Shawn Weeks (absolutesantaja)]

I wrote a little program to test this and the output may not be what you want. I feel like this has added too many quotes and still has an uneven number of single quotes.

file = open("output.txt","r")
fileString = file.read()
output = "'" + fileString.replace("'", "\'\"\'\"\'") + "'"
print output

Here is a small sample of the output.

SCSI_TPGS='"'"'0'"'"'
SCSI_TYPE='"'"'disk'"'"'
SCSI_VENDOR='"'"'SanDisk'"'"''"'"'
SCSI_VENDOR_ENC='"'"'SanDisk'"'"''"'"'
SCSI_MODEL='"'"'Cruzer_Fit'"'"'
SCSI_MODEL_ENC='"'"'Cruzer\x20Fit\x20\x20\x20\x20\x20\x20'"'"'
SCSI_REVISION='"'"'1.00'"'"'
ID_TYPE='"'"'disk'"'"'

review: Needs Information

[Ryan Harper (raharper)]

Thanks. shlex.quote() is designed to make the string usable directly in shell, like so:

% lxc exec x1 bash
root@x1:~# lsb_release -rd
Description: Ubuntu 16.04.6 LTS
Release: 16.04
root@x1:~# python3
Python 3.5.2 (default, Apr 16 2020, 17:47:17)
[GCC 5.4.0 20160609] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import shlex
>>> def shlex_quote(v):
... return "'" + v.replace("'", "\'\"\'\"\'") + "'"
...
>>> value="SanDisk'"
>>> value
"SanDisk'"
>>> shlex.quote(value)
'\'SanDisk\'"\'"\'\''
>>> shlex_quote(value)
'\'SanDisk\'"\'"\'\''
>>> print(shlex.quote(value))
'SanDisk'"'"''
>>>
root@x1:~# echo 'SanDisk'"'"''
SanDisk'

[Shawn Weeks (absolutesantaja)]

Thanks for the better test. I didn't fully understand what shlex was aiming for. I see the correct output now.

#!/bin/bash

outputVar=$(
cat << EOF | python
fileString = "'SanDisk''"
output = "'" + fileString.replace("'", "\'\"\'\"\'") + "'"
print output
quit()
EOF
)

sh -c "echo $outputVar"

'SanDisk''

[Ryan Harper (raharper)]

This unittest is not exposing the path we're seeing. I suspect there's some python string stuff happening here, I'll likely need to dump udevadm info --query=property --export output into a file and provide the decoded file output into the return of the mocked subp to have it trigger what I see a runtime.

First we shouldn't shlex.quote() everything, that leaves shell-escape characters in the values we'd like to use as they are, DEVTYPE="disk" for example, instead of DEVTYPE="'disk'".

>>> shlex.split("'disk'")
['disk']
>>> shlex.split(shlex.quote("'disk'"))
["'disk'"]

[Shawn Weeks (absolutesantaja)]

> This unittest is not exposing the path we're seeing. I suspect there's some
> python string stuff happening here, I'll likely need to dump udevadm info
> --query=property --export output into a file and provide the decoded file
> output into the return of the mocked subp to have it trigger what I see a
> runtime.
>
> First we shouldn't shlex.quote() everything, that leaves shell-escape
> characters in the values we'd like to use as they are, DEVTYPE="disk" for
> example, instead of DEVTYPE="'disk'".
>
> >>> shlex.split("'disk'")
> ['disk']
> >>> shlex.split(shlex.quote("'disk'"))
> ["'disk'"]

You're more than welcome to use output.txt on the bug report It was made by just piping the output of udevadm to a text file.

[Ryan Harper (raharper)]

Heh, that's exactly what I did =)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:45c546d8162297bf75e20d2d9c73a849ce04cdfb
https://jenkins.ubuntu.com/server/job/curtin-ci/69/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/69/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/69/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/69/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/69/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/69//rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

Broadly speaking this looks good, thanks for your work! I do have a few comments inline (though none of them are enormous).

review: Needs Fixing

[Ryan Harper (raharper)]

Thanks

a9c7968... by Ryan Harper on 2020-04-28

Use pipes.quote for py27

cf07a93... by Ryan Harper on 2020-04-28

Rework ValueError, update comment, conslidate trimmed value use

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:cf07a935f62457dfd8f474710f038e90e51135e2
https://jenkins.ubuntu.com/server/job/curtin-ci/70/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/70/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/70/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/70/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/70/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/70//rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

On Tue, Apr 28, 2020 at 04:53:30PM -0000, Ryan Harper wrote:
> > +try:
> > + shlex_quote = shlex.quote
> > +except AttributeError:
> > + # python2.7 shlex does not have quote, give it a try
> > + def shlex_quote(value):
>
> Yes, that seems *much* better. Thanks for tracking it down.

No problem!

> > + # strip the leading/ending single tick from udev output
>
> I'll rework.

Thanks!

> > + self.assertEqual('SanDisk'"'"'', info['SCSI_VENDOR'])
>
> What's wrong with matching the exact output from shlex_quote() ?
>
> % python3
> Python 3.8.2 (default, Mar 13 2020, 10:14:16)
> [GCC 9.3.0] on linux
> Type "help", "copyright", "credits" or "license" for more information.
> >>> import shlex
> >>> shlex.quote("SanDisk'")
> '\'SanDisk\'"\'"\'\''
> >>> print(shlex.quote("SanDisk'"))
> 'SanDisk'"'"''
> >>>

I don't think that's what you're doing:

    In [4]: 'SanDisk'"'"'' == shlex.quote("SanDisk'")
    Out[4]: False

'SanDisk'"'"'' is equivalent to `'SanDisk' + "'" + ''`, which:

    In [5]: 'SanDisk'"'"'' == "SanDisk'"
    Out[5]: True

effectively undoes the escaping. This is a little easier to see in
triple quotes (where all the double and single quotes do not need
escaping, and I added newlines and a .strip() for further clarity):

    In [13]: """
        ...: 'SanDisk'"'"''
        ...: """.strip() == shlex.quote("SanDisk'")
    Out[13]: True

5ceb185... by Ryan Harper on 2020-04-28

Rework unittest and document expected result, being careful with quoting

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:5ceb185c76810d3052190b73451cd3e02479a966
https://jenkins.ubuntu.com/server/job/curtin-ci/71/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/71/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/71/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/71/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/71/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/71//rebuild

review: Approve (continuous-integration)

[Dan Watkins (oddbloke)]

LGTM, thanks! I've updated the commit message for the pipes change, but will leave it to you to mark Approved.

review: Approve

diff --git a/curtin/udev.py b/curtin/udev.py
index e2e3dd0..f83f216 100644
--- a/curtin/udev.py
+++ b/curtin/udev.py
@@ -4,7 +4,14 @@ import shlex
 import os
 
 from curtin import util
-from curtin.log import logged_call
+from curtin.log import logged_call, LOG
+
+try:
+    shlex_quote = shlex.quote
+except AttributeError:
+    # python2.7 uses pipes.quote
+    import pipes
+    shlex_quote = pipes.quote
 
 
 def compose_udev_equality(key, value):
@@ -90,7 +97,23 @@ def udevadm_info(path=None):
             value = None
         if value:
             # preserve spaces in values to match udev database
-            parsed = shlex.split(value)
+            try:
+                parsed = shlex.split(value)
+            except ValueError:
+                # strip the leading/ending single tick from udev output before
+                # escaping the value to prevent their inclusion in the result.
+                trimmed_value = value[1:-1]
+                try:
+                    quoted = shlex_quote(trimmed_value)
+                    LOG.debug('udevadm_info: quoting shell-escape chars '
+                              'in %s=%s -> %s', key, value, quoted)
+                    parsed = shlex.split(quoted)
+                except ValueError:
+                    escaped_value = (
+                        trimmed_value.replace("'", "_").replace('"', "_"))
+                    LOG.debug('udevadm_info: replacing shell-escape chars '
+                              'in %s=%s -> %s', key, value, escaped_value)
+                    parsed = shlex.split(escaped_value)
             if ' ' not in value:
                 info[key] = parsed[0]
             else:
diff --git a/tests/data/udevadm_info_sandisk_cruzer.txt b/tests/data/udevadm_info_sandisk_cruzer.txt
new file mode 100644
index 0000000..a605afe
--- /dev/null
+++ b/tests/data/udevadm_info_sandisk_cruzer.txt
@@ -0,0 +1,54 @@
+DEVPATH='/devices/pci0000:00/0000:00:1d.0/usb2/2-1/2-1.5/2-1.5.1/2-1.5.1:1.0/host6/target6:0:0/6:0:0:0/block/sdc/sdc1'
+DEVNAME='/dev/sdc1'
+DEVTYPE='partition'
+PARTN='1'
+MAJOR='8'
+MINOR='33'
+SUBSYSTEM='block'
+USEC_INITIALIZED='5265867'
+SCSI_TPGS='0'
+SCSI_TYPE='disk'
+SCSI_VENDOR='SanDisk''
+SCSI_VENDOR_ENC='SanDisk''
+SCSI_MODEL='Cruzer_Fit'
+SCSI_MODEL_ENC='Cruzer\x20Fit\x20\x20\x20\x20\x20\x20'
+SCSI_REVISION='1.00'
+ID_SCSI='1'
+ID_VENDOR='SanDisk_'
+ID_VENDOR_ENC='SanDisk\x27'
+ID_MODEL='Cruzer_Fit'
+ID_MODEL_ENC='Cruzer\x20Fit\x20\x20\x20\x20\x20\x20'
+ID_REVISION='1.00'
+ID_TYPE='disk'
+ID_SCSI_INQUIRY='1'
+ID_VENDOR_ID='0781'
+ID_MODEL_ID='5571'
+ID_SERIAL='SanDisk__Cruzer_Fit_4C530000140118216265-0:0'
+ID_SERIAL_SHORT='4C530000140118216265'
+ID_INSTANCE='0:0'
+ID_BUS='usb'
+ID_USB_INTERFACES=':080650:'
+ID_USB_INTERFACE_NUM='00'
+ID_USB_DRIVER='usb-storage'
+ID_PATH='pci-0000:00:1d.0-usb-0:1.5.1:1.0-scsi-0:0:0:0'
+ID_PATH_TAG='pci-0000_00_1d_0-usb-0_1_5_1_1_0-scsi-0_0_0_0'
+ID_PART_TABLE_UUID='36b64baf'
+ID_PART_TABLE_TYPE='dos'
+ID_FS_UUID='2020-04-23-08-02-07-00'
+ID_FS_UUID_ENC='2020-04-23-08-02-07-00'
+ID_FS_BOOT_SYSTEM_ID='EL\x20TORITO\x20SPECIFICATION'
+ID_FS_VERSION='Joliet Extension'
+ID_FS_LABEL='Ubuntu-Server_20.04_LTS_amd64'
+ID_FS_LABEL_ENC='Ubuntu-Server\x2020.04\x20LTS\x20amd64'
+ID_FS_TYPE='iso9660'
+ID_FS_USAGE='filesystem'
+ID_PART_ENTRY_SCHEME='dos'
+ID_PART_ENTRY_UUID='36b64baf-01'
+ID_PART_ENTRY_TYPE='0x0'
+ID_PART_ENTRY_FLAGS='0x80'
+ID_PART_ENTRY_NUMBER='1'
+ID_PART_ENTRY_OFFSET='0'
+ID_PART_ENTRY_SIZE='1859584'
+ID_PART_ENTRY_DISK='8:32'
+DEVLINKS='/dev/disk/by-path/pci-0000:00:1d.0-usb-0:1.5.1:1.0-scsi-0:0:0:0-part1 /dev/disk/by-id/usb-SanDisk__Cruzer_Fit_4C530000140118216265-0:0-part1 /dev/disk/by-label/Ubuntu-Server\x2020.04\x20LTS\x20amd64 /dev/disk/by-partuuid/36b64baf-01 /dev/disk/by-uuid/2020-04-23-08-02-07-00'
+TAGS=':systemd:'
diff --git a/tests/unittests/test_udev.py b/tests/unittests/test_udev.py
index 33d5f44..919c7c0 100644
--- a/tests/unittests/test_udev.py
+++ b/tests/unittests/test_udev.py
@@ -1,14 +1,18 @@
 # This file is part of curtin. See LICENSE file for copyright and license info.
 
 import mock
+import shlex
 
-from curtin import udev
+from curtin.udev import (
+        udevadm_info,
+        shlex_quote,
+        )
 from curtin import util
 from .helpers import CiTestCase
 
 
 UDEVADM_INFO_QUERY = """\
-DEVLINKS='/dev/disk/by-id/nvme-eui.0025388b710116a1'
+DEVLINKS='/dev/disk/by-id/nvme-eui.0025388b710116a1 /dev/disk/by-id/nvme-n1'
 DEVNAME='/dev/nvme0n1'
 DEVPATH='/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/nvme/nvme0/nvme0n1'
 DEVTYPE='disk'
@@ -24,7 +28,8 @@ USEC_INITIALIZED='2026691'
 """
 
 INFO_DICT = {
-    'DEVLINKS': ['/dev/disk/by-id/nvme-eui.0025388b710116a1'],
+    'DEVLINKS': ['/dev/disk/by-id/nvme-eui.0025388b710116a1',
+                 '/dev/disk/by-id/nvme-n1'],
     'DEVNAME': '/dev/nvme0n1',
     'DEVPATH':
         '/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/nvme/nvme0/nvme0n1',
@@ -48,17 +53,49 @@ class TestUdevInfo(CiTestCase):
         """ udevadm_info returns dictionary for specified device """
         mypath = '/dev/nvme0n1'
         m_subp.return_value = (UDEVADM_INFO_QUERY, "")
-        info = udev.udevadm_info(mypath)
+        info = udevadm_info(mypath)
         m_subp.assert_called_with(
             ['udevadm', 'info', '--query=property', '--export', mypath],
             capture=True)
         self.assertEqual(sorted(INFO_DICT), sorted(info))
 
+    @mock.patch('curtin.util.subp')
+    def test_udevadm_info_escape_quotes(self, m_subp):
+        """verify we escape quotes when we fail to split. """
+        mypath = '/dev/sdz'
+        datafile = 'tests/data/udevadm_info_sandisk_cruzer.txt'
+        m_subp.return_value = (util.load_file(datafile), "")
+        info = udevadm_info(mypath)
+        m_subp.assert_called_with(
+            ['udevadm', 'info', '--query=property', '--export', mypath],
+            capture=True)
+
+        """
+        Replicate what udevadm_info parsing does and use pdb to examine what's
+        happening.
+
+        (Pdb) original_value
+        "SanDisk'"
+        (Pdb) quoted_value
+        '\'SanDisk\'"\'"\'\''
+        (Pdb) split_value
+        ["SanDisk'"]
+        (Pdb) expected_value
+        "SanDisk'"
+        """
+        original_value = "SanDisk'"
+        quoted_value = shlex_quote(original_value)
+        split_value = shlex.split(quoted_value)
+        expected_value = split_value if ' ' in split_value else split_value[0]
+
+        self.assertEqual(expected_value, info['SCSI_VENDOR'])
+        self.assertEqual(expected_value, info['SCSI_VENDOR_ENC'])
+
     def test_udevadm_info_no_path(self):
         """ udevadm_info raises ValueError for invalid path value"""
         mypath = None
         with self.assertRaises(ValueError):
-            udev.udevadm_info(mypath)
+            udevadm_info(mypath)
 
     @mock.patch('curtin.util.subp')
     def test_udevadm_info_path_not_exists(self, m_subp):
@@ -66,4 +103,4 @@ class TestUdevInfo(CiTestCase):
         mypath = self.random_string()
         m_subp.side_effect = util.ProcessExecutionError()
         with self.assertRaises(util.ProcessExecutionError):
-            udev.udevadm_info(mypath)
+            udevadm_info(mypath)