FRRouting / frr

The FRRouting Protocol Suite
https://frrouting.org/
Other
3.31k stars 1.25k forks source link

OSPFv3 authentication trailer allows MD5, which goes against RFC 7166 #13806

Closed dpward closed 6 months ago

dpward commented 1 year ago

8890 allows the selection of MD5 as a hash algorithm in the OSPFv3 authentication trailer. That is not valid according to RFC 7166:

The currently valid algorithms (including mode) for OSPFv3
Cryptographic Authentication include:

o  HMAC-SHA-1,

o  HMAC-SHA-256,

o  HMAC-SHA-384, and

o  HMAC-SHA-512.

Of the above, implementations of this specification MUST include
support for at least HMAC-SHA-256 and SHOULD include support for
HMAC-SHA-1 and MAY also include support for HMAC-SHA-384 and
HMAC-SHA-512.

Implementations of this specification MUST use HMAC-SHA-256 as the
default authentication algorithm.

I can't see anywhere in the original issue or PR that this was discussed or considered. I believe this should be deprecated and removed. If there is justification for supporting the use of MD5 in ospf6d, then it needs to be clearly documented that this is non-standard behavior.

Related to this, we don't follow the last paragraph above: we force the user to select an authentication algorithm, without defaulting to HMAC-SHA-256. So without knowing better, the user may just pick the first algorithm in this list, which happens to be MD5: https://github.com/FRRouting/frr/blob/1a5607eab984d64810aa47f9fcb57a8f8aeb9923/ospf6d/ospf6_interface.c#L2953-L2968

rameshabhinay commented 1 year ago

When the draft was published, I remember that there was support for MD5. when I had implemented it long back in Huawei. So the support was added if old implementations need interop. We can mention in document that we should prefer HMAC algorithms.

dpward commented 1 year ago

When the draft was published, I remember that there was support for MD5. when I had implemented it long back in Huawei.

I went back to the first draft and each one after that, and I couldn't find any mention of MD5. Are you sure you're not getting this mixed up with RFC 5709?

So the support was added if old implementations need interop..

Can we identify which implementations those are please? I haven't been able to find another one yet that supports MD5, and I think we want to avoid a situation where this is the only one. (I found a message on Slack from someone who attempted to configure MD5 on a Mikrotik router but found it not to work, and it's unclear that it was ever meant to: the same CLI commands support both OSPFv2 and OSPFv3.)

Or to put it differently: all implementations have been required to support HMAC-SHA-256 since the first draft. Are there any which do not?

riw777 commented 1 year ago

I believe MD5 was supported in the early drafts, but removed because of concerns around the relative weakness of MD5. I think it's reasonable to raise an issue to remove MD5 support at some point in the future. We would need to go through the normal deprecation process, though.

dpward commented 1 year ago

Deprecating it makes the most sense to me.

(For the record - I searched every draft of RFC 6506 and RFC 7166, without finding any mention of MD5. I only see it left over in OSPFv2.)

dpward commented 1 year ago

@riw777 This is just adding a deprecation warning when someone tries to use the command right? Really wish we had gotten this into 9.0...

riw777 commented 1 year ago

We would normally allow the old command for a year or so, but for this case I think it's okay just to add a deprecation message because this is security related.

dpward commented 1 year ago

@riw777 Are you working on this? The issue was self-assigned to you. I believe this is a very quick change?

github-actions[bot] commented 6 months ago

This issue is stale because it has been open 180 days with no activity. Comment or remove the autoclose label in order to avoid having this issue closed.

frrbot[bot] commented 6 months ago

This issue will be automatically closed in the specified period unless there is further activity.

dpward commented 6 months ago

@frrbot Not resolved, still valid, kindly reopen