Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authenticode verification support for Windows (similar to the 'signature' table on macOS) #3716

Merged
merged 9 commits into from Oct 14, 2017

Conversation

@alessandrogario
Copy link
Contributor

@alessandrogario alessandrogario commented Sep 21, 2017

Goal

This PR aims to add Authenticode verification support to osquery in a similar way to the 'signature' virtual table that has been implemented for macOS.

Changes

I have defined a signature virtual table for Windows, containing the following columns: path, original_program_name (from the publisher), serial_number, issuer_name, subject_name, result.

The following are the possible values for the result column:

  • missing: Missing signature.
  • invalid: An invalid signature, caused by missing or broken files.
  • untrusted: A signature that could not be validated.
  • distrusted: A valid signature, explicitly distrusted by the user.
  • valid: A valid signature which is not explicitly trusted by the user.
  • trusted: A valid signature, trusted by the user.

Examples

Verifying the authenticode signature for running executables

SELECT process.pid, process.path, authenticode.result
FROM processes as process
LEFT JOIN signature AS authenticode
ON process.path = authenticode.path;
...
| 5124 | C:\Windows\system32\winlogon.exe    | missing |
| 2476 | C:\Windows\system32\dwm.exe         | missing |
| 2744 | C:\Windows\system32\fontdrvhost.exe | trusted |
| 6372 | c:\windows\system32\sihost.exe      | missing |
| 7152 | c:\windows\system32\svchost.exe     | trusted |
| 80   | c:\windows\system32\svchost.exe     | trusted |
| 6748 | c:\windows\system32\taskhostw.exe   | trusted |
...

Listing unsigned/untrusted processes listening for connections

SELECT port_info.pid, port_info.family, port_info.protocol, port_info.port, port_info.address,
       process.name, process.path,
       authenticode.result AS authenticode

FROM listening_ports AS port_info

LEFT JOIN processes
AS process ON port_info.pid = process.pid

LEFT JOIN signature AS authenticode
ON process.path = authenticode.path

WHERE authenticode <> "trusted";


+------+--------+----------+-------+---------+-------------+------------------------------------+--------------+
| pid  | family | protocol | port  | address | name        | path                               | authenticode |
+------+--------+----------+-------+---------+-------------+------------------------------------+--------------+
| 6588 | 2      | 6        | 80    | 0.0.0.0 | hfs.exe     | C:\Program Files (x86)\HFS\hfs.exe | missing      |
| 1724 | 2      | 6        | 49667 | 0.0.0.0 | spoolsv.exe | C:\Windows\System32\spoolsv.exe    | missing      |
| 1724 | 23     | 6        | 49667 | ::      | spoolsv.exe | C:\Windows\System32\spoolsv.exe    | missing      |
+------+--------+----------+-------+---------+-------------+------------------------------------+--------------+
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 21, 2017

@alessandrogario has updated the pull request. View: changes

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 21, 2017

@alessandrogario has updated the pull request. View: changes

Copy link
Member

@theopolis theopolis left a comment

I'd recommend creating an authenticode or signature table for Windows.

@alessandrogario
Copy link
Contributor Author

@alessandrogario alessandrogario commented Sep 24, 2017

@theopolis sure! I'm still writing the utilities I need, as I also want to be able to list signature information like signer name etc. Once it's done, I'll create the new table; how would you like it to work? Should it just list the running executables?

@theopolis
Copy link
Member

@theopolis theopolis commented Sep 25, 2017

how would you like it to work? Should it just list the running executables?

Check out the signature table for macOS, if you don't mind, mimic the user experience for that.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 28, 2017

@alessandrogario has updated the pull request.

1 similar comment
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 28, 2017

@alessandrogario has updated the pull request.

@alessandrogario
Copy link
Contributor Author

@alessandrogario alessandrogario commented Sep 28, 2017

I have defined a 'signature' virtual table for Windows; it only sports two columns for now, but I plan on adding more. I'm using a more verbose field instead of the 'signed' boolean used in the macOS implementation.

The following are the possible states:

  • missing: Missing signature.
  • invalid: An invalid signature, caused by missing or broken files.
  • untrusted: A signature that could not be validated.
  • distrusted: A valid signature, explicitly distrusted by the user.
  • valid: A valid signature which is not explicitly trusted by the user.
  • trusted: A valid signature, trusted by the user.

EDIT: I should probably put this information in the summary!

Copy link
Contributor

@muffins muffins left a comment

A couple of smaller nits and a question about the << operator definition, not sure if that'll fly as it seems risky to define it in such a high scope for something used by so many places in the code base.

Otherwise super excited for this table to land!

case SignatureInformation::Result::Untrusted: {
row["result"] = "untrusted";
break;
}

This comment has been minimized.

@muffins

muffins Sep 29, 2017
Contributor

perhaps you could default to unknown or -1?

This comment has been minimized.

@alessandrogario

alessandrogario Sep 29, 2017
Author Contributor

Done!

return row;
}

Status toUtf16String(std::wstring& output, const std::string& s) {

This comment has been minimized.

@muffins

muffins Sep 29, 2017
Contributor

We have a stringToWstring function already implemented here that you could use, but this looks good -- any chance you could take a look at our implementation to see if it could stand some improvements? ;)

This comment has been minimized.

@alessandrogario

alessandrogario Sep 29, 2017
Author Contributor

They look similar! I am now using that one!

if (status.ok()) {
results.push_back(r);
} else {
LOG(ERROR) << status.getMessage();

This comment has been minimized.

@muffins

muffins Sep 29, 2017
Contributor

ERROR seems a bit intense for a failure to compute the signature of an executable, maybe this could be a WARNING instead? I think we try and reserve ERROR for something which would halt the daemon itself.

])
implementation("signature@generateQueryResults")
examples([
"select * from signature where path = 'C:\\Windows\\notepad.exe'",

This comment has been minimized.

@muffins

muffins Sep 29, 2017
Contributor

Would you mind adding in an example that joins off of the process table just to show up the extra cool features? :D

This comment has been minimized.

@alessandrogario

alessandrogario Sep 29, 2017
Author Contributor

Done! I've added a simple example in the table specs, and also edited the PR summary to include a couple of more (that we can probably add to the docs once this is merged).

Result result;
};

Row& operator<<(Row& row, const SignatureInformation& signature_info) {

This comment has been minimized.

@muffins

muffins Sep 29, 2017
Contributor

While this is pretty awesome how you're making use of the << on Rows, I'm honestly not sure how to feel about it as it seems dangerous to overload an operator on a data structure that gets used in so many places in this high of namespace - /CC @theopolis to potentially put my worries to rest, but are there potentially other (arguably less awesome) ways to achieve the same behavior?

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 29, 2017

@alessandrogario has updated the pull request. View: changes

@alessandrogario alessandrogario changed the title [WIP] Authenticode verification support for the 'processes' table on Windows [WIP] Authenticode verification support for Windows (similar to the 'signature' table on macOS) Sep 29, 2017
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 30, 2017

@alessandrogario has updated the pull request. View: changes

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 30, 2017

@alessandrogario has updated the pull request. View: changes

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 30, 2017

@alessandrogario has updated the pull request. View: changes

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 30, 2017

@alessandrogario has updated the pull request. View: changes

@alessandrogario alessandrogario changed the title [WIP] Authenticode verification support for Windows (similar to the 'signature' table on macOS) Authenticode verification support for Windows (similar to the 'signature' table on macOS) Sep 30, 2017
@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Sep 30, 2017

@alessandrogario has updated the pull request.

Copy link
Member

@theopolis theopolis left a comment

I'll defer to @muffins but a quick note, some of the VLOGs seem a bit generic.


namespace osquery {
// These are defined in osquery/core/windows/wmi.cpp
std::wstring stringToWstring(const std::string& src);

This comment has been minimized.

@theopolis

theopolis Oct 4, 2017
Member

Are these not in a header somewhere?

This comment has been minimized.

@muffins

muffins Oct 5, 2017
Contributor

@alessandrogario - Just in case you didn't see it they're here :)

// clang-format on

#include <osquery/core.h>
#include <osquery/core/conversions.h>

This comment has been minimized.

@theopolis

theopolis Oct 4, 2017
Member

This should be in it's own include block (double new line separated) and included using the local search:

#include "osquery/core/conversions.h"
return Status(0, "Ok");
}

QueryData generateQueryResults(QueryContext& context) {

This comment has been minimized.

@theopolis

theopolis Oct 4, 2017
Member

Can you use a more specific symbol name here? genAuthenticode or something similar?

@muffins muffins dismissed their stale review Oct 4, 2017

Dismissing as my review is dated. Will review again after the changes requested by @theopolis are addressed.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Oct 5, 2017

@alessandrogario has updated the pull request.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Oct 5, 2017

@alessandrogario has updated the pull request. View: changes

&certificate_context->pCertInfo->SerialNumber;

std::stringstream serial_number_string;
for (DWORD i = serial_number->cbData - 1; i < serial_number->cbData; i--) {

This comment has been minimized.

@theopolis

theopolis Oct 10, 2017
Member

Can you explain this loop?

This comment has been minimized.

@alessandrogario

alessandrogario Oct 10, 2017
Author Contributor

This is used to print the serial number backwards (as it is shown in the Authenticode dialog properties of Windows).

Since DWORD is unsigned, the loop uses the fact that according to the standard the value will wrap around once you decrement past zero.

return false;
}

std::string buffer;

This comment has been minimized.

@theopolis

theopolis Oct 10, 2017
Member

You can create the buffer with a requested container size in the ctor

@theopolis
Copy link
Member

@theopolis theopolis commented Oct 10, 2017

ok to test

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 10, 2017

🙅 The commit 50f6de6 (Job results: 2631) failed the code audit or documentation test.

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 10, 2017

👎 The commit 50f6de6 (Job results: 5988) failed one or more tests (Linux).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 10, 2017

👎 The commit 50f6de6 (Job results: 5989) failed one or more tests (Linux).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 10, 2017

👎 The commit 2e99777 (Job results: 5991) failed one or more tests (Linux).

Column("issuer_name", TEXT, "The certificate issuer name"),
Column("subject_name", TEXT, "The certificate subject name"),
Column("result", TEXT, "The signature check result")
])

This comment has been minimized.

@theopolis

theopolis Oct 12, 2017
Member

Can we morph fields from darwin or windows such that we can have a unified signature table?

schema([
    Column("path", TEXT, "Must provide a path or directory", required=True),
    Column("signed", INTEGER, "1 If the file is signed else 0"),
    Column("identifier", TEXT, "The signing identifier sealed into the signature"),
    Column("cdhash", TEXT, "SHA1 hash of the application Code Directory"),
    Column("team_identifier", TEXT, "The team signing identifier sealed into the signature"),
    Column("authority", TEXT, "Certificate Common Name"),
])
  • path cool!
  • original_program_name can this be called identifier on both?
  • serial_number, issuer_name, subject_name can be extended schema on Windows for now.
  • cdhash does this checksum have an authenticode equivalent?
  • result can stay on both, maybe on darwin we fill this in with "signed" or "unknown"
  • signed being a boolean should be included here, it should indicate if the binary is properly signed.

This comment has been minimized.

@theopolis

theopolis Oct 12, 2017
Member

The reason that the build is breaking in the CI is because there are two specfiles named signature. This is not allowed.

This comment has been minimized.

@alessandrogario

alessandrogario Oct 12, 2017
Author Contributor

original_program_name: I have renamed this to "identifier".
cdhash: There is something but to grab it you need to parse the PE header and locate the PKCS#7 structure that contains the hash. There is really no easy way to calculate it without a custom tool (and it's hard to use this value in a meaningful way) so I skipped it.
result: I'll add this column to the darwin schema, and set it to "signed" when "signed" is 1 and to "unknown" when it is 0.
signed: Should this value be 1 only when the signature is both valid and trusted?

What should I do with the remaining fields? Should both specfiles (darwin and Windows) contain the exact same columns?

This comment has been minimized.

@theopolis

theopolis Oct 13, 2017
Member

Leave the fields blank if there are no meaningful compromises/merging options.

You can add "extended" schema like:

extended_schema(WINDOWS, [
    Column("friendly_name", TEXT, "The friendly display name of the interface."),
])

The first option is the requirement. These columns will be set to HIDDEN on platforms that do not match. Such that a SELECT * does not return them.

Your options for evaluators right now are: WINDOWS, LINUX, POSIX, DARWIN, FREEBSD. They are small functions defined in gentable.py.

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Oct 13, 2017

@alessandrogario has updated the pull request.

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

🙅 The commit 6e5b348 (Job results: 2647) failed the code audit or documentation test.

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 6e5b348 (Job results: 6006) failed one or more tests (Linux).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 6e5b348 (Job results: 923) failed one or more tests (FreeBSD).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 6e5b348 (Job results: 6007) failed one or more tests (Linux).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 6e5b348 (Job results: 924) failed one or more tests (FreeBSD).

@facebook-github-bot
Copy link

@facebook-github-bot facebook-github-bot commented Oct 13, 2017

@alessandrogario has updated the pull request.

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 087f1fb (Job results: 925) failed one or more tests (FreeBSD).

@osqueryer
Copy link

@osqueryer osqueryer commented Oct 13, 2017

👎 The commit 087f1fb (Job results: 926) failed one or more tests (FreeBSD).

@theopolis theopolis added approved and removed ready for review labels Oct 14, 2017
@theopolis
Copy link
Member

@theopolis theopolis commented Oct 14, 2017

LGTM, @muffins I'll let you have the final look and merge when you're ready.

@muffins
Copy link
Contributor

@muffins muffins commented Oct 14, 2017

LGTM

@muffins muffins merged commit e888f3e into osquery:master Oct 14, 2017
4 of 5 checks passed
4 of 5 checks passed
FreeBSD Build finished.
Details
Code Audit Build finished.
Details
Linux Build finished.
Details
Windows Build finished.
Details
macOS/OS X Build finished.
Details
trizt added a commit to trizt/osquery that referenced this pull request May 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.