Revert "Improve detection & handling of duplicate Node ID:"

[intelliot]

High Level Overview of Change

This reverts commit 5a15229, which was introduced in 1.10.0-b1 and is not present in the code that is currently running in production (version 1.9.4).

Notice that this PR, #4439, is a "smaller" change than #4438 from the perspective of what node operators will be getting when they upgrade from 1.9.4.

Context of Change

Alternative to #4438.

Given that 1.9.4 is stable and not vulnerable, the safest and most conservative change would be no change at all.

Therefore, we should consider the option of simply reverting the problematic commit, which is what this does. While git did not report any conflicts, the revert should still be code reviewed to ensure no unintended consequences.

Only one of {self, #4438} should be merged, a decision ideally based on a consensus of the maintainers.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

Test Plan

@manojsdoshi @sgramkumar to prepare a test plan.

---

Acknowledgements:
Aaron Hook for responsibly disclosing this issue.

Bug Bounties and Responsible Disclosures:
We welcome reviews of the rippled code and urge researchers to
responsibly disclose any issues they may find.

To report a bug, please send a detailed report to:

    bugs@xrpl.org

[nbougalis]

I think that reverting the change is riskier than just removing the call that forces the server to stop—a one line fix, literally.

I, personally, prefer merging #4438 which fixes this issue and avoids changing any code that doesn’t need to be touched.

[RichardAH]

Agree with Nik here, why revert already reviewed and committed code unrelated to the actual attack vector just because it was in the same commit? That doesn't strike me as a sensible thing to do.

[intelliot]

@RichardAH :

1. The original reviews were clearly insufficient, as they failed to catch a critical vulnerability.
2. 100% of the network is currently running version 1.9.4, which is the latest stable release of rippled. 5a15229 was added in 1.10.0-b1, and is not present in 1.9.4. Version 1.10.0 will contain 70+ other commits, many of which are also risky. Reverting 5a15229 would give a result that has "minimal" changes versus what's currently live in production.

Does that make sense?

@nbougalis Why do you think reverting the change is "riskier"?

I can certainly see an argument that it is "no safer", if you have 100% confidence that the other changes in 5a15229 are beneficial and contain no bugs.

Ultimately, it really depends on whether there are any changes in the reverted commit that you feel must be included in 1.10.0. (What changes, specifically, are those?)

From Aaron Hook: aside from the stop signal, it also switched to using a cryptographically secure prng for the cookie, and allowed hex encoded values in addition to base64. I'd probably lean towards doing the revert and if any of the other changes are important enough, add them back as a new patch. But I don't have the context for the original patch, so I don't feel strongly either way.

[intelliot]

I, personally, prefer merging #4438 which fixes this issue and avoids changing any code that doesn’t need to be touched.

I understand this point of view. However, by the same argument, it is worth nothing that this PR (#4439) fixes this issue and, compared to 1.9.4 (which is live in production) avoids changing any code that doesn't need to be touched.

[RichardAH]

1. The original reviews were clearly insufficient, as they failed to catch a critical vulnerability.

I think this is not a very strong argument. The same review process applies to the rest of the commits in the repository for literally years. If the review process is untrustworthy then all commits are.

[intelliot]

Canceled in favor of #4438