reject ipv4 strings with an octet with a leading zero

[karenetheridge]

Ensuring implementations reject these values will help guard against this
security vulnerability.
see https://sick.codes/universal-netmask-npm-package-used-by-270000-projects-vulnerable-to-octal-input-data-server-side-request-forgery-remote-file-inclusion-local-file-inclusion-and-more-cve-2021-28918/

[karenetheridge]

thread: https://old.reddit.com/r/perl/comments/mfzxuh/security_issues_in_perl_ip_address_distros/

[Julian]

@karenetheridge looking back at this, I'm actually not convinced it's correct now personally.

The spec link (in e.g. Draft 7) links to RFC 2673, which simply says:

dotted-quad = decbyte "." decbyte "." decbyte "." decbyte

and then

Each number represented by a must be between 0 and 255, inclusive.

But that's all really, so it doesn't seem these are in fact invalid under the RFC to me. What part of the RFC should outlaw these (and is that part in fact cited for the JSON Schema spec?)

[karenetheridge]

It's ambiguous, because many systems interpret numbers with leading digits to be octal, not decimal, but not all.

Regardless of what happens in JSON Schema I will not be allowing octal values in ipv4 addresses in my implementations because of the grave security risks.

[Julian]

I think that's reasonable to do as an implementer, but I don't think an implementation that doesn't follow it is noncompliant (so unless I'm missing something, it seems we should back out the invalid test here) -- e.g. I was looking at what Python claims to do when trying to implement this, which is that it allows leading zeros as long as they're not ambiguous between decimal and octal apparently.

(EDIT: In Python-land, it looks like discussion of this is ongoing actually).

EDIT2: I also see the language in RFC3986 which talks about this -- but which isn't referenced by the JSON Schema spec. So ISTM if we want this behavior we should add a reference to that?

[karenetheridge]

It's frequently come up that our references to RFCs and other spec documents are out of date or not quite accurate, so I think we need to think carefully about what to do about this in the future -- perhaps have specific language like "if a referenced RFC is obsoleted by a new RFC, that new document shall be used", which prevents us from locking into known-incorrect or potentially even harmful behaviour.

It's ironic that this comes up at the same time as my observation that the draft4 spec specifically contradicts the test suite (regarding the behaviour of "additionalItems" with a schema") and yet in that case we are deciding to ignore the spec and instead test what is considered to be more correct.

So, when do we ignore the literal wording of the spec and instead do what is right, and when do we not?

[Julian]

It's frequently come up that our references to RFCs and other spec documents are out of date or not quite accurate, so I think we need to think carefully about what to do about this in the future -- perhaps have specific language like "if a referenced RFC is obsoleted by a new RFC, that new document shall be used", which prevents us from locking into known-incorrect or potentially even harmful behaviour.

I'm not sure what I think about this from the spec's perspective, I agree with you this has happened a few times before, but regardless I as usual don't think it's for the test suite to make that kind of call, so if that's the kind of thing we go with obviously the suite should follow, but until then I think it definitely shouldn't.

I'll have to read your draft4 example, I'm not sure I've seen it. Broadly, the way I tend to think about contradictions are:

• if a specification is clear and unambiguous about a particular behavior, and the test suite contradicts it, the relevant tests MUST be changed regardless of how long they've been present (reasoning being that's a bug, and it doesn't matter how long it's existed)
• if a specification is ambiguous -- meaning multiple different interpretations are possible, each reasonably likely to have been intended at the time of authorship of the specification, then the test suite MUST NOT take a position on which interpretation is correct by adding required tests that impose one or the other, but MAY choose to include multiple different, even conflicting, optional files which allow implementers to pick one or the other interpretation
• if a specification is ambiguous but tests already exist in the suite, and have existed for long periods of time (years, or since the date of the specification) then they SHOULD be left in -- this one is probably the contentious one, though I've "cited" it previously, but yeah this one I can imagine folks disagreeing with -- the reason I think we should do that is because implementers who've implemented the suite and may have done so desiring the behavior tested in it under the ambiguity (i.e. who care for one particular resolution to the ambiguity) would have no means to notice or realize that their test coverage has disappeared from under them once the suite removes the tests -- and we have no means to notify them really. So if they've been in there long enough, and no one complains for 3 years, and all of a sudden someone realizes or points out another reading of the spec, then we can add new optional tests for the alternate reading, but we should not move the tests, for fear of breaking implementations which rely on it as above.

Here to me though this seems like the first case -- I don't see a way to read it such that this behavior is intended. But even if you claim it's the third case, the difference to me would be these are new tests, not long standing ones.

Obviously others may have other perspectives (either about the general framework above or about this specific change) so probably we should codify something (e.g. maybe alongside the stuff in #439 ?) or solicit other opinions about the change itself?

[karenetheridge]

Regarding these tests in particular (behaviour of octal digits in the ipv4 format) I'd say I would be okay with moving them to optional/, but format tests are already optional, so I'm not sure what else we can do.

[Julian]

Oh, yeah that's fair -- hm, ok not sure what a good way to differentiate them is but we've run into this before bleh. OK, maybe they stay as-is then for lack of a better idea.