search

securitytxt / security-txt

A proposed standard that allows websites to define security policies.
https://securitytxt.org
Other
1.78k stars 68 forks source link

Address AD review feedback #165

Closed EdOverflow closed 4 years ago

EdOverflow commented 4 years ago

Hi all,

As promised, here are my review comments on the draft. Most of them are just suggestions (I tried to be explicit when changes are definitely required), and I'm happy to talk more about any of them if they're unclear or undesired. (There's also some questions without suggestion, where we might want to be more clear or I needed some more information.)

Section 1.1

Many security researchers encounter situations where they are unable to report security vulnerabilities to organizations because there is no course of action laid out or no way indicated to contact the owner of a particular resource.

I'm not sure whether this works better as an "or" or an "and".

In this document, we define a richer, machine-parsable and more extensible way for organizations to communicate information about their security disclosure policies, which is not limited to email and also allows for additional features such as encryption. This format is designed to help assist with the security disclosure process by making it easier for organizations to designate the preferred steps for researchers to take when trying to reach out to them with security vulnerabilities.

Is it worth saying something about being able to reach out confidentially, specifically (e.g., for a critical security vulnerability that would be embargoed)?

Section 3

This text file contains multiple directives with different values. The "directive" is the first part of a field all the way up to the colon ("Contact:") and follows the syntax defined for "field-name" in section 3.6.8 of [RFC5322]. Directives MUST be case-insensitive (as per section 2.3 of [RFC5234]). The "value" comes after the directive

I think it would be okay to just have a declarative statemnet that "Directives are case-insensitive (per [...]", since 5322 references 5234 and that's the authoritative spec.

unlimited number of fields. It is important to note that you MUST have a separate line for every field. One MUST NOT chain multiple

It's pretty uncommon to have "you" in RFCs; a possible rewording would be "It is important to note that each field MUST appear on its own line". I'm happy to provide suggested rewordings for the other instances in the document on demand, but will otherwise not call out subsequent occurrences.

values for a single directive unless it is explicitly defined by that particular field. Unless otherwise indicated in a definition of a

and similarly, "Unless specified otherwise by the field definition, multiple values MUST NOT be chained together for a single directive." (which also avoids the "MUST [...] unless" construction that some IESG members dislike).

Section 3.3

The bits about one field per line are largely duplicating the toplevel Section 3 content; I'd consider just making this section be "Line separator" and just have the line separator convention.

Section 3.5.3

This directive allows you to provide an address that researchers SHOULD use for reporting security vulnerabilities. The value MAY be

This is probably fine as a regular "should" -- if someone's reading/using this document, they're going to be using the indicated contact information. If they're not using this document, then nothing we say is relevant to them.

Section 3.5.4

researchers SHOULD use for encrypted communication. You MUST NOT directly add your key to the field, instead the value of this field MUST be a URI pointing to a location where the key can be retrieved from. If this directive indicates a web URL, then it MUST begin with

nit: this is a comma splice. Also, the "from" at the end of the sentence is probably not necessary.

Section 3.5.7

The order in which they appear MUST NOT be interpreted as an indication of priority - rather these MUST BE interpreted as all being of equal priority.

"MUST BE" is not an RFC 8174 keyword; just "MUST be" should be fine.

Section 3.7

My inclination would be to generate a throwaway PGP key and generate an actual signature using that key over the file, rather than just using the implicit "[signature]" as filler. But if you have a preference here, it outweights mine :) (Also, GnuPG v1 is considered "legacy" by upstream.)

Section 4.1

as per [RFC8615]. A security.txt file located under the top-level path SHOULD either redirect (as per section 6.4 of [RFC7231]) to the security.txt file under the /.well-known/ path or be used as a fallback if the ".well-known" path cannot be used.

I think we need to be careful about how we describe this behavior; BCP 190 affirms that the URL namespace of a site belongs to the site owner and not the authors of protocols; /.well-known/ is the only significant carveout from that. So anything (even SHOULD-level) that's trying to make decisions about behavior outside of /.well-known/ is likely to get pushback from the ART ADs. We may be best with a declarative statemnet like "for legacy compatibility, a security.txt file might be placed at the top-level path or redirect to the well-known path".

If retrieval of a "security.txt" file from the top-level path results in a redirect (as per section 6.4 of [RFC7231]), the implementors MUST NOT follow that redirect if it leads to another domain or subdomain but SHOULD follow that redirect within the same domain name (but not different subdomain on the same domain).

Why does this restriction on following redirects only apply to /security.txt and not /.well-known/security.txt ? (I see the prohibition is repeated in Section 6.1 as well, though again without discussion of the differences between paths.)

Section 4.3

I don't think I know what an "internal host" is. Is this supposed to be directly on a filesystem or via http(s) access?

Section 5

I expect/hope that we can get some advice from an ABNF expert to tighten this up a little, especially the 'unsigned' construction. But I'm not suggesting that we make any changes at this time, since we're as likely to break things as improve them.

Section 6

Implementors SHOULD review this section as well as the security considerations section of [RFC8615].

The "SHOULD" seems redundant with BCP 72; I'd suggest "In addition to the security considerations of [RFC8615], the following considerations apply."

Section 6.2

Do we want to note that "Not having a security.txt file at all is preferable to having one with stale information in it"?

Section 6.3

I pondered a bit what sort of "attackers" are relevant for this scenario; it seems that this applies to both a compromised site (where a researcher that detects the compromise will try to use security.txt and is directly attacked by the file) and fully malicious sites that host malformed security.txt files to catch anyone that happens to be looking. It's sometimes helpful to give the reader additional clarity on what the "attacker" might be in cases like this, though I don't think it's stricly necessary to do so here.

Also, a couple of nits (missing articles) "The ABNF grammar" and "The same concerns apply".

Section 6.4

The presence of a security.txt file can be interpreted by researchers as providing permission to do security testing against that asset.

I suggest s/can/might/, since we do not want people to do this, and the "can" form sounds like it is giving permission. (Similarly for the rest of the paragraph.)

Section 6.5

This text sounds like it's reserving "security.txt" at all directory levels, not just the root. (It's also pretty likely to get pushback on BCP 190 grounds.)

Section 6.6

Implementors MUST also perform the correct TLS verification (as per [RFC6125]).

When using RFC 6125 we still need to specify how to authenticate the server, e.g., DNS-ID must match the name in the URI from security.txt. (Maybe allow CN-ID as well, but DNS-ID is preferred these days.)

Section 7.2

  1. The document in which the specification of the field is published

If this is mandatory, then the registration procedure should be "Specification Required" (which also includes review by experts). If not, then we should add something like "(if available)".

  1. New or updated status, which MUST be one of: current: The field is in current use deprecated: The field is in current use, but its use is discouraged historic: The field is no longer in current use

It looks like this was intended to be formatted as a list?

An update may make a notation on an existing registration indicating that a registered field is historical or deprecated if appropriate.

If we're going to allow updates to existing entries, then the registry needs to also track the change controller for registered values. The IESG would be appropriate for the initial registry contents.

Section 8

nit: I don't think there's a hyphen in "SECDISPATCH".

Thanks,

Ben

nightwatchcyber commented 4 years ago

Addressed in the -09 draft