Update linux block_device and disk_encryption source data to simple sysfs implementation #8182
Conversation
Would this implementation also allow |
I could definitely look at making that possible. |
Micah-Kolide
changed the title
…nd block devices tables
Micah-Kolide
force-pushed
the
micah/modify_source_of_disk_encryption_data
branch
from
e542a40
to
5eee8db
Compare
|
@Smjert I think you said you could review this PR this week? |
Yes I began looking into it today but other things got my priority; I'll be able to finish my review by eod tomorrow. |
|
|
||
| struct crypt_device* cd = nullptr; | ||
| auto ci = crypt_status(cd, name.c_str()); | ||
| auto ci = crypt_status(cd, r["name"].c_str()); |
There was a problem hiding this comment.
Minor nit: for performance, prefer using the value you have copied from (block_device.name) instead of fetching it back from the std::map.
| @@ -50,15 +42,16 @@ void genFDEStatusForBlockDevice(const Row& block_device, | |||
| r["encryption_status"] = kEncryptionStatusEncrypted; | |||
| r["type"] = ""; | |||
|
|
|||
| auto crypt_init = crypt_init_by_name_and_header(&cd, name.c_str(), nullptr); | |||
| auto crypt_init = | |||
| crypt_init_by_name_and_header(&cd, r["name"].c_str(), nullptr); | |||
| if (crypt_init < 0) { | ||
| VLOG(1) << "Unable to initialize crypt device for " << name; | ||
| VLOG(1) << "Unable to initialize crypt device for " << r["name"]; |
| break; | ||
| } | ||
|
|
||
| struct crypt_active_device cad; | ||
| if (crypt_get_active_device(cd, name.c_str(), &cad) < 0) { | ||
| VLOG(1) << "Unable to get active device for " << name; | ||
| if (crypt_get_active_device(cd, r["name"].c_str(), &cad) < 0) { |
| if (crypt_get_active_device(cd, name.c_str(), &cad) < 0) { | ||
| VLOG(1) << "Unable to get active device for " << name; | ||
| if (crypt_get_active_device(cd, r["name"].c_str(), &cad) < 0) { | ||
| VLOG(1) << "Unable to get active device for " << r["name"]; |
| } | ||
| } | ||
| std::map<std::string, Row> encryption_status; | ||
| auto query_context = context.constraints.find("name")->second.getAll(EQUALS); |
There was a problem hiding this comment.
I would just access the value with [] here, since if whatever reason the value doesn't exist, it's undefined behavior when dereferencing the end iterator. Even better would actually be checking that the element actually exists, but we have using [] as a bit of a pattern it seems, because either we assume it's always there, and/or because std::map specifically will create an empty element if that key doesn't exist.
It's not ideal to rely on it though.
| // even when we are not going to return the parents, so that we can inherit | ||
| // model, serial, and vendor for non root block devices. | ||
| if (!block_device.parent.empty()) { | ||
| genBlockDeviceResult(block_devices[block_device.parent], |
There was a problem hiding this comment.
Please check that block_device.parent actually exists in the map, with find and checking against the end iterator.
| // Restore device path. | ||
| block_device.path = name == "size" | ||
| ? block_device.path.parent_path() | ||
| : block_device.path.parent_path().parent_path(); |
There was a problem hiding this comment.
I would suggest to use a copy of block_device.path which is thrown away at every loop and append the relative path to that.
This way I feel it's slightly clearer and doesn't need to make assumptions on the relative path depth to correctly restore the block_device.path.
| if (!block_device.parent.empty()) { | ||
| block_device.model = block_devices[block_device.parent].model; | ||
| block_device.serial = block_devices[block_device.parent].serial; | ||
| block_device.vendor = block_devices[block_device.parent].vendor; |
There was a problem hiding this comment.
Please don't assume the key exists; also access it only once.
| } | ||
|
|
||
| // Remove newlines from data. | ||
| data.erase(std::remove(data.begin(), data.end(), '\n'), data.cend()); |
There was a problem hiding this comment.
I think we should trim leading or following spaces if we want to keep consistency with the old visualization.
For instance for a QEMU CD-ROM device, the vendor and model columns have additional spaces as suffix:
select hex(vendor), hex(model) from block_devices where name = "/dev/sr0";
+------------------+----------------------------------+
| hex(vendor) | hex(model) |
+------------------+----------------------------------+
| 51454D5520202020 | 51454D552043442D524F4D2020202020 |
+------------------+----------------------------------+
Fixes #8033
Purpose of this PR is to remove Linux's
disk_encryptionreliance onblock_devices. We want a list of all block devices by name and a way to access their parent device, so I walk sysfs for that.libudev accesses block devices primarily in the same way, but with a lot of other options. Those options aren't utilized in osquery's current implementation. Since we don't use them I figured I could take out libudevs usage in our Linux
block_deviceanddisk_encryptiontable generations.I've added
block_device_enumerationas a fairly simple traversal of sysfs to access all of the relevant data we previously were collecting. This should also be easy to add to if other data is desired down the road.