[Swan-dev] ikev1_close_message()

D. Hugh Redelmeier hugh at mimosa.com
Sun Aug 26 21:25:32 UTC 2018


| From: Paul Wouters <paul at nohats.ca>
| Date: Sun, 19 Aug 2018 15:11:27 -0400 (EDT)

| On Sat, 18 Aug 2018, D. Hugh Redelmeier wrote:
| 
| > So these calls are wrong.

In other words: it is a bug in our code.

We can fix it by simply deleting these calls.

| I agree, but the question is what interop issues would be create by
| changing this? Would we break interop with our older self?

No.  This would affect output only and not change what we accept.

(If I'm wrong, the test suite would show us before anyone could get
hurt.)

| Or create
| interop code to deal with it?

No, I don't imagine that it is needed.

| And how about all the other (now frozen)
| ikev1 stacks?

It should not.  Of course I cannot promise that.  If it does, those
implementations are buggy.

| Would be reduce interoperability or increase it?

There is a chance that it would increase interoperability: we'd be in
conformance with the "standard".

Note that this bug only affects xauth.  Xauth doesn't even have a
standard, as you pointed out.  The relevant RFCS were not adopted and
were allowed to expire.

David McCullough's patch ab763468da8d8f33d5c7ac3d7281f704d60aed7d of
2013 Oct 1 adds a flag to suppress padding.  This suppresses both IKE
message padding and the xauth misuse of the padding routine.  It was
meant to help interop with Checkpoint and perhaps other unnamed
implementations.  It is imaginable that what the Checkpoint
implementation disliked was exactly what I dislike.  Unfortunately,
too little is recorded about this for me to tell.

I'd really like to test with the bug fixed and David's patch reversed.
But know no way of testing the result against Checkpoint (as far as I
know).  We don't even know which versions of Checkpoint are
problematic.

This is exactly why we need regression tests.  But then we'd have to
have our own copy of Checkpoint.

| Since
| we have no known reports of interoperability failure, I think it is
| more prudent to not change this anymore.

Perhaps.  But it bugs me.  After all, it is a bug in our code that
we're letting set there.


More information about the Swan-dev mailing list