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

Added sql512 and deprecated sql57 based on #153 #179

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

vanderpol
Copy link
Member

Attached are sample results from the SCC application, which has implemented this test.
SQL512_Sample_Results.zip

These results are from a SCAP datastream, and contain more than SQL512 tests, but there are plenty of SQL512 tests to demonstrate it's functionality.

This method is generating results for the entire host. IF/when SCAP 3.0 implements a 'target' method, results could then be generated based on host/instance/database.

@vanderpol vanderpol requested a review from solind October 16, 2024 13:53
@solind
Copy link

solind commented Oct 16, 2024

I don't think it's clear what the distinction is between the "instance" and "database" entities, as the documentation presupposes meanings for these terms (their meanings are not immediately clear to me either), and in your example the items all share the same instance "SQLEXPRESS" and the database value of "" (empty string).

Do you want to add meanings or examples to the documentation? My guess is that an "instance" means a distinct running instance of the DBMS, a server process listening on a distinct TCP port (i.e., a "SID" on Oracle). Whereas a "database" denotes something like a "tablespace" on Oracle.

@vanderpol
Copy link
Member Author

Good point @solind, I was just re-using the schema files from a previous developer, and should have put more into these new data elements. I'm no SQL Server/Oracle guru either, but from my view the following are true:

  • instance = The DBMS management system itself and any databases that it may contain. This could be a separate installation of binaries, or just a set of running processes used to manage the DBMS.
  • database = A specific database name within the DMBS system, where a database is a collection of tables

MS documentation on databases: https://learn.microsoft.com/en-us/sql/relational-databases/databases/databases?view=sql-server-ver16

MS documentation on database engine instances:
https://learn.microsoft.com/en-us/sql/database-engine/configure-windows/database-engine-instances-sql-server?view=sql-server-ver16

Oracle Instance docs:
https://docs.oracle.com/en/database/oracle/oracle-database/21/cncpt/oracle-database-instance.html#GUID-67247052-CE3F-44D2-BA3E-7067DEF4B6D5

Oracle Instance vs Database
https://stackoverflow.com/questions/17409862/database-vs-tablespace-whats-the-difference

Maybe instead of 'instance' we should use 'dbms_instance' and instead of 'database' say 'database_name'?

I'm open to all suggestions, and will attach some more robust content that uses both 'instance' and 'database' more than just .* and configure my test system to have a couple SQL instances on it.

@solind
Copy link

solind commented Oct 17, 2024

Personally, I think "instance" and "database" would be good enough, provided we have better schema documentation and examples.

@vanderpol
Copy link
Member Author

I've revised the instance and database documentation, I'm guessing it will need further refinement, but figured this was a start. I've attached some sample content and results. This content is only for SQL Server at the moment, although SCC supports Oracle as well, I just don't have an Oracle DBMS system available right now. We have plans to add support for PostgreSQL and MariaDB in the future. The attached test content is a small subset of the DISA STIG content for SQL DB and SQL Instance. When I find more free time, I may make some less DISA specific test content.
Second_Sample_Results_SQL512.zip
sql512_test_content.xml.txt

Copy link

@solind solind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably document what we mean by default when database@xsi:nil="true". If it's okay to mean that it's the default for whatever user is used to log in, then we should just say that. But also, then... in that case, should we have a username element in the sql512_item?

<xsd:documentation>When a pattern or string is entered, the OVAL interpeter will consider any matching instance as in scope for analysis.</xsd:documentation>
</xsd:annotation>
</xsd:element>
<xsd:element name="database" type="oval-def:EntityObjectStringType" nillable="true">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, what does it mean if xsi:nil="true"? (ETA - another comment below)

For that matter, what if the database doesn't have a "Tablespace" type of concept (e.g., Derby DB -- which isn't supported in OVAL, but I happen to know it doesn't have such a concept)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if xsi:nil = 'true', we just run the query against the DBMS instance, not a single specific database. I was debating the whole 'default' database terminology as it was confusing to me as well. Sometimes these queries are against a specific 'master' database, but other queries are are very bizzare, and may be SQL Server centric, but maybe other DBMS's have similar things, see below:

SELECT
SERVERPROPERTY('IsClustered') AS [is_clustered],
SERVERPROPERTY('IsHadrEnabled') AS [is_hadr_enabled]

This queries some configuration setting for the SQL Server instance, and is not targeted at any single database table.

<xsd:annotation>
<xsd:documentation>The database entity defines the specific database name to be used when connecting to the specified instance, where a database is defined as a collection of tables within a DBMS instance.</xsd:documentation>
<xsd:documentation>When a pattern or string is entered, the OVAL interpeter will perform the query against any matching databases.</xsd:documentation>
<xsd:documentation>If the xsi:nil attribute is set to true, then only the target instance's default database will be queried.</xsd:documentation>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duh, read ahead, right? But still, in Oracle, for instance, there's a default Tablespace that can be defined for each user, but I don't believe there's like a universally "default" database defined.

So if xsi:nil="true", it's going to depend on the user who is used to log in. And the user is going to be left up to the tool, just like for OS scanning.

Copy link
Member Author

@vanderpol vanderpol Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool will determine which credential to use, just like with OS scanning. I'll ponder how to better describe the xsi:nil use case, although it may only be an SQL Server scenario, until I get more familiar with other DBM's.

@vanderpol
Copy link
Member Author

vanderpol commented Oct 17, 2024

We should probably document what we mean by default when database@xsi:nil="true". If it's okay to mean that it's the default for whatever user is used to log in, then we should just say that. But also, then... in that case, should we have a username element in the sql512_item?

Adding a username will make the content only be useful to a single server, unless there are generic usernames that are present on all Oracle databases that you would know about in advance? Given that we would want to digitally sign the content, our goal would be content that could be used on 'all' database systems without being edited.

I think the term 'default' just needs to be removed and state that xsi:nil means that the OVAL interpreter will only run the query once, and against the dbms instance. For some queries this could also be accomplished by putting "master" or whatever database contains the configuration data for the instance as the database. The oddball queries as mentioned previously which select server properties, but don't seem to get data from any specific table really are the only queries that need this 'nil' value.

@solind
Copy link

solind commented Oct 17, 2024

@vanderpol I was not suggesting adding a username to the definitions, but to the collected system-characteristics (which are by nature specific to a single server). I recall Joval put the login username somewhere in the results; I don't recall if XCCDF results had a specific place for that, or if we put it into a metadata field.

@vanderpol
Copy link
Member Author

@solind, thanks for the clarification. I could see where knowing what username/credentials were used to scan the target dbms, as we save the credentials for scanning the OS as part of XCCDF results such as <cdf:identity authenticated="true" privileged="true">DESKTOP-7ODDPV4\Jack</cdf:identity> I'm not sure if there's a place in any OVAL results to store identiy/credentials? I know we don't currently.

@solind
Copy link

solind commented Oct 17, 2024

@vanderpol That was the place! The cdf:identity field!

I don't think there's a formal OVAL equivalent, but there are plenty of "metadata" fields that can have xs:any children. I guess you're suggesting the database user could just be stored in one of those places?

@vanderpol
Copy link
Member Author

vanderpol commented Oct 17, 2024

Your idea of a username/identity as part of the collected item is interesting and I'm wondering why we don't have an identity globally in the oval results and pondering if we should. It wouldn't replace the need for at the SQL item level. If there was enough interest, I'd say we do both in a standard method. Using generic metadata in not where I would recommend.

@vanderpol vanderpol closed this Oct 17, 2024
@vanderpol vanderpol reopened this Oct 17, 2024
@vanderpol
Copy link
Member Author

Accidentally closed, fat fingers on my phone..

@vanderpol
Copy link
Member Author

In updating the documentation I realized that the 'version' design from the former developer was well intentioned, but over simplified, converting 'real' versions to human readable versions (example for SQL Server 2016, the real version is something like 13.0.6450.1. I'll be updating sample content next week to replace the version of '2016' with a pattern match of ^13.0..*

This method will allow tools and content authors to use 'standard', well documented versions for each product, even if they are not quite a easy to understand or correlate to their end user visible versions.

@vanderpol
Copy link
Member Author

@solind Sorry for the extra rounds of documentation and test content review. In writing documentation and attempting to explain it to you, I realized that the existing SQLEXT spec, which I used to create the SQL512 was definitely under specified, and the version field being translated likely won't age well vs just using the 'real' version.

@solind
Copy link

solind commented Oct 18, 2024

@vanderpol Yikes, good catch! No worries, I'll watch out for that commit. ... or did you do that already?

@vanderpol
Copy link
Member Author

@solin I committed it, but am wondering if I should be making the version field a version data type. Will research that next week

@vanderpol
Copy link
Member Author

@solind any thoughts on "version" being a stringType vs a versionType? It's been string for both the 'sql' and 'sql57' test (not that they were ever really used). I can see benefits for each. As the data is pretty disparate across vendors, a string is more forgiving and flexible, where as versionType allows for less than, greater than etc... and I think should still work with non-numeric data such as '10.4.7-MariaDB'

I have it updated in my code to use versionType, but would like a sanity check before committing.

@solind
Copy link

solind commented Oct 21, 2024

@vanderpol Indeed, the "string" datatype is much more forgiving. Most databases use traditional version schemes, but it's not always clear what deserves to be thought of as the "version".

Take a favorite like MS SQL. Is the version "SQL Server 2022", or is it "16.0.4135.4" (which they call the "build number") -- or is it "16" or "16.0"?

IIRC, with Joval, we compared the supplied string value using engine-specific logic, to determine whether we had an available compatible driver to make the connection. So, I'd probably just keep it a string, until we can better define what exactly we're looking for.

Copy link

@solind solind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wasn't the documentation actually more specific/clear before making this change?

@vanderpol
Copy link
Member Author

vanderpol commented Oct 22, 2024

I removed a schematron rule which I didn't create (former developer on our team wrote it), and didn't understand. and a couple other items were abbreviated a bit. If you have any specific concerns, I'll pull back the previous commit.

@solind
Copy link

solind commented Oct 22, 2024

Huh, when I wrote that last comment of mine the changes consisted solely of the removal of some doc annotations from independend-definitions-schema.xsd. Now the changes look great! Weird.

@vanderpol
Copy link
Member Author

Thanks @solind, as always your feedback is very much appreciated!

@vanderpol vanderpol merged commit 8163611 into develop Oct 23, 2024
@vanderpol vanderpol deleted the 153-add-independent-sql512-test-deprecating-sql57 branch October 29, 2024 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants