Ban unicode control characters from utf8 fields - #1341
Conversation
morehouse
left a comment
There was a problem hiding this comment.
Does it also make sense to call out that the receiving node MAY reject gossip with non-conforming alias or features fields?
Eclair and LND in particular may want to reject such gossip during decoding since they currently don't preserve the underlying bytes of some non-conforming fields for signature verification (ACINQ/eclair#3314, lightningnetwork/lnd#10835).
|
Good idea, I added explicit requirements on the receiving side in f160681 |
tnull
left a comment
There was a problem hiding this comment.
I do wonder if it would be worth to provide test vectors for this? Maybe they could be generated by reusing the script we recently added to LDK, see https://github.com/lightningdevkit/rust-lightning/pull/4605/changes#diff-af90f55a5f5e4af60b13fc942aee60e49311cb223f15bccdf373117b4aabc3df
As discussed in lightning#1260, we should ban NULL and other control characters in utf8 fields, along with characters that don't provide any value in node aliases: there's no valid use-case for them. Unfortunately, unicode cannot provide static lists of characters in each category, since it is open to extension. We thus more strongly restrict writers, and set reader restrictions that are more stable. We also make sure that feature bits MUST be minimally-encoded everywhere instead of being lenient for `node_announcement` for no good reason.
f160681 to
dcd957d
Compare
|
Should probably do the same for BOLT 11/12 descriptions/memos/etc. |
Probably a follow up PR, but I wonder if we should expand this to any |
Those test vectors contain a valid signature. Only the alias is invalid.
Note that we already require that features are minimally-encoded.
And require that features are minimally-encoded.
I've added a few test vectors in 403f409. I don't think we can or should be exhaustive, we only need to nudge implementers to make sure they've banned the right categories, but if you want me to add other specific test vectors, don't hesitate to generate them and I'll add them! |
I agree, some of those already mentioned that features must be minimally-encoded. I've added explicit requirements where they were missing in e686eef and b10ea05. As a rule of thumb, features must always be minimally encoded. It's just that we initially didn't include this requirement in the very early days of lightning and wanted to preserve backwards-compatibility while implementations ensured that they minimally encoded features. Nowadays, every implementation should have long shipped versions that minimally encode all the time, so we can reject anything that isn't minimally encoded. |
|
I assume you've run a quick pass on the current gossip db to make sure ~everything is minimal? |
|
Yes, I haven't found any gossip where features are not minimally encoded on mainnet today.
They're rather small nodes (but not inactive) except for the last one, with has 570,000,000 sats (~370,000$) of public capacity (8 channels). If anyone knows who owns these nodes, please get them to change their alias, otherwise their node announcement will stop propagating soon! Or maybe I messed up my eclair code and I'm too trigger-happy in which case it would be nice to have another implementation test those announcements: |
|
Makes sense to standardize the strictness. As far as the nodes with now invalid aliases, I think we'll go with a route where we'll silently re-write the the alias to be compliant with these new rules. |
Wouldn't rewriting the alias invalidate the |
What roasbeef means here is not that intermediate nodes would rewrite the |
|
Sorry, yesterday I was not able to speak up during the meeting due to a microphone issue! I think that if this can impact the protocol, every implementation should do a check during the startup and emit a warning or not start at all if this can impact channel availability. In my experience, some node operators do not pay attention to the node status (now it should be easier with AI tho) Otherwise Concept ACK for me |
|
cACK |
We ban `NULL` and other control characters in utf8 fields: there's no valid use-case for them. We also make sure that feature bits MUST be minimally-encoded everywhere instead of being lenient for no good reason. See lightning/bolts#1341
|
While reviewing LDK's U+2028 LINE SEPARATOR ( Suggest also banning |
Thanks, good catch! Can you provide a test vector for this? I'll add it with the removed of |
|
The test from that branch was |
|
Here is a commit you can cherry-pick: vincenzopalazzo@94de118 It sits on top of b10ea05 and does two things:
{
"name": "Alias containing a line separator (Zl)",
"alias": "lightning\u2028rocks",
"announcement": "0101e7965f97f2e32a81eff5f58936b1d201b3c62f33c67d383ecf5b7cdba062fd0f6357d2914084953042ec18f30ae37cf3a65ac2a5839337b53004043398c7d88a000067b64b00034f355bdcb7cc0af728ef3cceb9615d90684bb5b2ca5f859ab0f0b704075871aa0102036c696768746e696e67e280a8726f636b730000000000000000000000000000000000"
},
{
"name": "Alias containing a paragraph separator (Zp)",
"alias": "lightning\u2029rocks",
"announcement": "0101911380dd0f32972e3d4cf79973b8d857966069538517e93bf9a543ed540763037195800c1f33b45ed908dbe45b66e08941ea61c6e9247a2d03e459ea5127f761000067b64b00034f355bdcb7cc0af728ef3cceb9615d90684bb5b2ca5f859ab0f0b704075871aa0102036c696768746e696e67e280a9726f636b730000000000000000000000000000000000"
}Same construction as the existing four: I included the rule text change in the same commit so it stays self-consistent, since the vectors are "MUST ignore" and without |
As discussed in #1260, we should ban NULL and other control characters in utf8 fields: there's no valid use-case for them.
We also make sure that feature bits MUST be minimally-encoded everywhere instead of being lenient for
node_announcementfor no good reason.