05:58:13 <br-m> <rbrunner7> Left a comment there. IMHO we soon may need some simple rules of thumbs how we deal with PRs that are about clear edge cases that maybe don't merit our time.
06:00:42 <br-m> <rbrunner7> And really, now I wait for the next AI coming up with an idea for an exploit based on the new behavior of dropping unassigned code points, characters we have no idea about what they are. Passphrases that transport whole trojan binaries, for example :)
06:32:17 <br-m> <torir:unredacted.org> Safest approach is to encode whichever version of the Unicode library we are using into the wallet seed, so that we can use the same normalization when decrypting with the passphrase. Then we just have to include every version of the Unicode library we've ever used, forever. That way lies madness.
06:32:55 <br-m> <torir:unredacted.org> Next best approach is probably to warn people that their passphrase has characters that aren't supported, to let them know that an updated version of the app might normalize those characters in a way that breaks descrypting their wallet.
06:37:24 <br-m> <torir:unredacted.org> Granted, this is a rare edge case, unless people actually are using emoji in their passwords.
06:37:51 <br-m> <torir:unredacted.org> Or maybe are using a newly supported language in Unicode with special normalization behavior that our app doesn't know about yet?
08:15:29 <moneromooo> What code points are assigned is irrelevant. UTF-8 encoding is a well defined system and the semantics attached to any code point has nothing to do with the bit string representation.
08:17:15 <moneromooo> Now, if some I/O software monero uses and can't do without barfs at some code points, it might be worth corralling but it would just be a bodge.
08:18:03 <moneromooo> If something sends you UTF-16 or UCS2-LE, you just convert it, as we'd already have to do anyway.
08:19:07 <moneromooo> If the issue is one of output, that's not a problem, unicode has a "can't represent" glyph for this. If it's can't input, then ping upstream for a change.\
08:20:17 <moneromooo> Having some subset of unicode that some program supports is unwanted behaviour. Every program would have their own ghetto.
08:45:21 <br-m> <torir:unredacted.org> I think this is a normalization problem. The problem is in Unicode, one semantic character can have multiple valid bitstring representations. I can type in é as U+00E9 (LATIN SMALL LETTER E WITH ACUTE) or as U+0065 U+0301 (LATIN SMALL LETTER E and COMBINING ACUTE ACCENT). Normalization is supposed to convert all ways to repre [... too long, see https://mrelay.p2pool.observer/e/79LAt60LWTRLSzVq ]
08:45:50 <br-m> <torir:unredacted.org> Normalization works great, except when new Unicode characters are added and old programs don't know how to normalize the new characters. It's an awful edge case to have.
08:54:11 <br-m> <rbrunner7> Well, the PR at hand is not exactly about normalization problems. We now throw away code points we don't recognize, and the proposal on the table is to keep them, to "err on the safe side" as I understand it. Whether we even had to normalize the unrecognized code points if we only understood them is a problem of a whole different magnitude, and something that is probably impossible to solve.
08:55:24 <br-m> <rbrunner7> But as I tried to argue in my PR comment: If the official duty of a Monero dev from now on is to worry even about such edge cases, work may grind to a halt, and devs might get crazy. We have to find a workable middle way here.
08:56:38 <br-m> <torir:unredacted.org> Emoji in passwords has to be so unlikely in the first place, I don't think this is a priority at all either.
08:56:52 <br-m> <rbrunner7> Exactly :)
09:02:35 <moneromooo> OK, the combining system was a bad idea in some ways...
09:03:22 <br-m> <torir:unredacted.org> I think if the combining system was the original system in the first place, so normalization wasn't necessary, this wouldn't have been an issue.
09:03:31 <moneromooo> Still, low level, I'd treat these two bit strings as different.
09:03:54 <moneromooo> If they end up showing the exact same glyph at render time, this is not our problem.
09:04:32 <br-m> <torir:unredacted.org> I can imagine trying to open a wallet on my computer and on my phone, and having the two input methods produce two different bitstrings despite the same character on the keyboard.... I just avoid having any unicode text in my passwords if I can avoid it, to be honest.
09:14:50 <br-m> <jbabb:cypherstack.com> Emojis are an edge case, but if localized keyboards can trigger it then it’s not so much an edge case as an internationalization issue
09:16:41 <br-m> <torir:unredacted.org> Anyway, the Monero solution is correct here except for the very rare edge case of unrecognzied code points, which realistically we can't even do anything about, except maybe warn the user that it might have occurred. Since it is so rare, current concensus is to just ignore it.
15:30:36 <br-m> <thomasbuilds:matrix.org> it's best I share the AI findings on polyseed so we can evaluate these together, whether we care about them fixed or not...
15:30:36 <br-m> <thomasbuilds:matrix.org> > Medium
15:30:36 <br-m> <thomasbuilds:matrix.org> > 
15:30:36 <br-m> <thomasbuilds:matrix.org> > 1. zh_s/zh_t share 1275 chars at the same indices, so ~1 in 2,000 Chinese seeds hit ERR_MULT_LANG although both lists decode to the identical seed. Monero has no decode_explicit path (electrum-words.cpp:415), so such seeds from other wallets fail with "Electrum-style word list failed verification"; switching a wallet'[... more lines follow, see https://mrelay.p2pool.observer/e/79iMw60Lc1dFaGZR ]
15:39:13 <br-m> <thomasbuilds:matrix.org> as the AI says I started doing 3, also unsure whether to PR this or not
16:47:18 <br-m> <rbrunner7> Well, you presented point 3, that exception propagation problem on on 32 bit ARM already to me in a DM a few days ago, together with a possible solution that the AI brought up. I judge this, like that "emoji in passphrase" problem that we discussed earlier today here channel, to be a improbable edge case.
16:48:37 <br-m> <rbrunner7> Plus, the solution that the AI came up with was ugly as hell, and IMHO it would be a pain to carry this arround for years to come in the interest of an edge case on a platform that is almost obsolete already today.
16:50:27 <br-m> <rbrunner7> Thus, unless you come up with something better than the AI suggested as solution, I really don't think I will support such a PR.
16:50:45 <br-m> <rbrunner7> Of course I don't decide such things, but still.
16:52:50 <br-m> <jbabb:cypherstack.com> > <@rbrunner7> Well, you presented point 3, that exception propagation problem on on 32 bit ARM already to me in a DM a few days ago, together with a possible solution that the AI brought up. I judge this, like that "emoji in passphrase" problem that we discussed earlier today here channel, to be a improbable edge case.
16:52:50 <br-m> <jbabb:cypherstack.com> I have to check if this is actually the case, sorry--so this is a load-bearing "if" coming up ;)
16:52:50 <br-m> <jbabb:cypherstack.com> but if any user's default desktop or mobile keyboard can reproduce the issue then it's not an edge case :3
16:53:51 <br-m> <rbrunner7> I don't think that with regular keyboard input on any system you can produce invalid UTF-8 that will throw if you try to normalize it. You have to find some way to throw more or less garbage at that method.
16:54:35 <br-m> <rbrunner7> Maybe prepare something on another system, mail it to your 32 bit ARM smartphone, copy, paste, bang.
16:54:45 <br-m> <rbrunner7> Something invalid on purpose
16:55:22 <br-m> <rbrunner7> But as I said: Here we are, spending time, on things that IMHO don't merit it.
16:55:27 <br-m> <jbabb:cypherstack.com> gotcha, I was going off of the example above that you can have multiple identical-looking characters with different codes.  it only really applies if, like if Torir mentioned, some localized keyboard's characters don't match eg. an android keyboard's characters exactly.
16:55:39 <br-m> <jbabb:cypherstack.com> and yeah, as you say, probably doesn't merit the consideration :) just a nit
16:56:08 <br-m> <rbrunner7> Not sure you already fully understand exactly where the problem of 3. can occur, but never mind :)
16:56:31 <br-m> <jbabb:cypherstack.com> and/or it only matters after monero gets widely adopted in some country which triggers the case and polyseed gets adopted across repos and/or in an inconsistent way
16:57:53 <br-m> <jbabb:cypherstack.com> yes, sorry, identical-looking characters aren't the issue in 3., no
16:59:23 <br-m> <rbrunner7> I am also a bit sceptical about 4: How would you "warn" over RPC and in an API method? Ok, you could give back some boolean, but well, we now implement only minimal support for encrypted Polyseeds, so ...
17:00:48 <br-m> <rbrunner7> 2. is t the point where I start to fear that from now on AIs will bring up this point for all time to come, ad nauseam, because they don't "understand" we do what they find problematic fully on purpose and *by design
17:05:40 <br-m> <rbrunner7> By the way, I already commented to @thomasbuilds:matrix.org about point 2 here: https://github.com/monero-project/monero/pull/10765#discussion_r3943358250
17:06:16 <br-m> <rbrunner7> I explained my "Wont fix" to them already.
17:06:55 <br-m> <jpk68:matrix.org> Also, for 32-bit ARM, I'd say this is worth fixing, as we provide release binaries for that platform
17:08:13 <br-m> <rbrunner7> Sigh. Maybe you would change your mind if you saw the "solution". It's fugly. But anybody if free to PR anything, I am well aware, and maybe people will disagree.
17:08:40 <br-m> <rbrunner7> Maybe somebody should PR to stop building those 32-bit ARM binaries, don't really know ...
17:09:01 <br-m> <jpk68:matrix.org> Haha, that's definitely a solution
17:10:46 <br-m> <rbrunner7> I repeat: Almost for sure you have prepare UTF-8 fully outside of any regular string input on any reasonable system, with some "criminal" energy, and then somehow throw that at the software on any 32-bit ARM smartphone. And then, all that happens is that the app terminates.
17:10:56 <br-m> <rbrunner7> illegal UTF-8
17:11:40 <br-m> <jpk68:matrix.org> Another fix would be to immediately dial the police on the ARM smartphone, due to the use of illegal UTF-8 /s
17:12:09 <br-m> <rbrunner7> But hey, if that's one of our worries now, while people discuss ways to fully eclipse Monero nodes from the network over in the MRL meeting ...
17:15:35 <br-m> <rbrunner7> Point 1. is also a bit theoretical. But I can say there is somebody who said they will code some way into the CLI wallet to specify the language at restore time if the seed turns out to be ambiguous and language detection fails. If that person makes good on their stated intention, point 1. will get solved sooner or later.
17:21:00 <br-m> <rbrunner7> So we have, as I see it: 1. Solution forthcoming from somebody else hopefully, 2. All good, it's by design, we really want it this way, 3. edge case of an edge case, and 4. worth looking at, but not terribly important either, not a "medium"
17:21:56 <br-m> <rbrunner7> I have to willfully prevent myself from looking at the "low" stuff in any closer way ...
17:22:42 <br-m> <jpk68:matrix.org> Would you mind if I submitted a patch for wiping the PBKDF2 stack buffers? I think that would be good to have, plus would be a simple fix
17:24:49 <br-m> <rbrunner7> No. I saw that in an earlier review, had a quick look and asked myself whether it's possible at all to read out something from those buffers about any secret. But, well, yes, why not wipe it.
17:26:25 <selsta> I thought we planned to stop supporting 32 bit android
17:26:35 <selsta> afaik the binary never worked anyway
17:26:43 <selsta> or at least was not stable
17:26:50 <br-m> <rbrunner7> Haha, that would almost ironic
17:27:35 <selsta> https://github.com/monero-project/monero/pull/9528
17:27:40 <selsta> found it, they have been broken for years
17:28:32 <selsta> though see the discussion, others have different opinion
17:30:04 <br-m> <rbrunner7> Well, we are 2 years later
17:30:42 <br-m> <rbrunner7> And if no such device is running an Android version that still receives security updates from Google, maybe it would be our holy duty to shoot those builds down for good
17:36:11 <br-m> <rbrunner7> selsta: Now I am confused. That 9528 PR was merged. So exactly what did we stop to offer as official binary then?
17:36:30 <selsta> It was only merged to master branch.
17:36:45 <selsta> We do releases from release branch. Plan was to remove support with v0.19.
17:36:59 <br-m> <rbrunner7> Ah, I see.
17:37:03 <br-m> <jpk68:matrix.org> tevador: Not sure if you think this is worth fixing, but FWIW - "lang.c never wipes idx[16] (word indices stay on the stack after decode)."
17:37:04 <selsta> But so far we still plan to support it in code, just no more binaries.
17:37:12 <br-m> <jpk68:matrix.org> Ah, he's not here, nevermind.
18:25:36 <br-m> <thomasbuilds:matrix.org> I tried to improve it, you might want to check my patch-8 branch > <@rbrunner7> Thus, unless you come up with something better than the AI suggested as solution, I really don't think I will support such a PR.
18:27:07 <br-m> <thomasbuilds:matrix.org> sorry if I'm bringing up some of the stuff we talked about in DM, but I think it's best we take these publicly. It's best other see by themselves too. Polyseed PR being merged there is nothing that would slow down your work.
18:28:21 <br-m> <thomasbuilds:matrix.org> it's just about being conscious of these and willingly ignore/fix them
18:32:27 <br-m> <ofrnxmr> 32bit arm android monerod did not work > <selsta> though see the discussion, others have different opinion
18:33:39 <br-m> <ofrnxmr> I had like.. 5 different arm32 android devices, android tv, phone/ tablet, etc, and none of them could run monerod
18:35:19 <br-m> <ofrnxmr> across different manufactures and time production years. Not sure is monero-wallet-* worked
19:04:51 <br-m> <rbrunner7> @thomasbuilds:matrix.org: "there is nothing that would slow down your work" Sure, but I still carefully watch the general developing situation regarding AI reviews, far above and beyond this single Polyseed PR. I do see some probability that a crisis will develop around the question how to best handle edge cases.
19:06:32 <selsta> for larger changes we just need to make a cut and say something is good enough to merge and whatever issues remain get fixed later like we did for Polyseed
19:10:18 <br-m> <thomasbuilds:matrix.org> @rbrunner7: tbh there is not an infinity of edge cases AI finds. for example I vastly audited mx25519 and after the few PRs I submited, AI said everything is clean (as far as fable 5 is concerned). Same with randomX.
19:10:50 <br-m> <thomasbuilds:matrix.org> ofc these are different and involve less design question that polyseed for monero
19:11:10 <br-m> <thomasbuilds:matrix.org> but still
19:12:57 <br-m> <thomasbuilds:matrix.org> AI is unaware of the design decisions of monero, it's just a technical tool. if we drop 32 bit arm android it's good.
19:13:30 <br-m> <ofrnxmr> We are only dropping release binaries and still supporting arm32 at least for the wallet api
19:13:58 <br-m> <ofrnxmr> 99.9% sure the api still works, otherwise cake etc would have a ton of issue reports from users who run 32bit hardware
19:18:12 <br-m> <thomasbuilds:matrix.org> On 4 AI meant a doc note for seed_offset with polyseed in the RPC docs / wallet2_api.h, nothing at runtime. > <@thomasbuilds:matrix.org> it's best I share the AI findings on polyseed so we can evaluate these together, whether we care about them fixed or not...
19:21:28 <br-m> <thomasbuilds:matrix.org> after giving AI some awareness of the stuff said here
19:21:28 <br-m> <thomasbuilds:matrix.org> > On 3: agreed that invalid UTF-8 has to be crafted. The same path also throws on over-long non-ASCII input though: an encrypted seed with a Korean passphrase of ~30 words (70 syllables, >575 bytes after NFKD) already fails today on x86-64 with "Unicode normalization failed", and would abort on 32-bit ARM. Rare either way. If  [... too long, see https://mrelay.p2pool.observer/e/mZjaya0LcEtqVUpK ]
19:21:28 <br-m> <thomasbuilds:matrix.org> > On 2: not about storing the offset, that's by design, agreed. It's that the import doesn't check the seed derives the wallet's spend key, and seed then prints a phrase that restores a different wallet without a hint. Derive and compare on import would do.[... more lines follow, see https://mrelay.p2pool.observer/e/mZjaya0LcEtqVUpK ]
19:23:08 <br-m> <ofrnxmr> iirc, we dont create encrypted seeds
19:23:38 <br-m> <ofrnxmr> Only cake does (currently), we only support restoring them due to cake already creating them
19:23:54 <br-m> <ofrnxmr> Thats my understanding from history, not from reading the code
19:24:52 <br-m> <jpk68:matrix.org> Yeah, I had waited for the Polyseed PR to be merged to add seed encryption info to the docs repo. Info has already been added for legacy seeds; looks like I'll need to look into what "canonical" Polyseeds are supposed to do now
19:25:21 <br-m> <jpk68:matrix.org> *in terms of methods for seed encryption (or otherwise)
19:27:16 <br-m> <ofrnxmr> We dont use/support encryption in "our" code aiui. upstream polyseed left it up to the imagination whether downstream projects wanted to use encryption
19:27:49 <br-m> <ofrnxmr> Feather and most other wallets did not. cake did. But cake supports restoring non-encrypted seeds.
19:28:47 <br-m> <ofrnxmr> Feather, for example, cant restore a passphrase "encrypted" cake seed. Monero cli should be able to, but cli only creates wallets with passphrases with encrypted bit set to 0 - compatible with feather and co
19:29:35 <br-m> <ofrnxmr> Ideally, cake stops encrypting and then the whole ecosystem will support restoring them, but wont create them anymore
19:35:02 <br-m> <thomasbuilds:matrix.org> anways anyone feel free to pick up ideas AI said, I'm moving to the more "critical" stuff now lol.
19:35:02 <br-m> <thomasbuilds:matrix.org> I think the most important fix to address is 11366, which I already did
19:41:23 <br-m> <thomasbuilds:matrix.org> Might be worth opening a PR giving polyseed's C target -fexceptions and wiping the buffer in utf8_norm on the throw path
19:46:21 <br-m> <rbrunner7> About 2: " It's that the import doesn't check the seed derives the wallet's spend key". My 2 cents: It's not an import. The AI just hallucinates an "import" here. And I would argue it's not our job to test whether Feather Wallet created a wallet where seed and spend private key match.
19:48:49 <br-m> <rbrunner7> I think our discussions here become somewhat hard to follow, and hard to understand. If you still think that the AI is onto something important with that point 2, something we might have to act on, maybe a good idea to create an issue, where we can properly discuss, and where I will be more than happy to explain further what t [... too long, see https://mrelay.p2pool.observer/e/tbO-yq0LOWgxa3JR ]
19:54:44 <br-m> <rbrunner7> "compile polyseed's C files with -fexceptions"  Hmmm, did the AI already mention this earlier, or did that only appear now, after doing another round? Anyway, if we can do with a simple switch that won't harm any modern ARM targets, nor any others, that's certainly much nicer than some ugly code modifications.
19:55:40 <br-m> <rbrunner7> @thomasbuilds:matrix.org: The problem solved by 11366 definitely looks like a good find!
20:15:42 <br-m> <jpk68:matrix.org> Looks like Polyseed code (namely the PBKDF2 part) throws some warnings with clang++
20:31:56 <br-m> <thomasbuilds:matrix.org> @thomasbuilds:matrix.org: @rbrunner7:monero.social
20:31:56 <br-m> <thomasbuilds:matrix.org> > with gcc and clang on x86-64, and gcc on aarch64 and armv7, the compiled code of polyseed's C files is byte-identical with and without -fexceptions. On x86-64 even the unwind tables are identical, and aarch64 emits them by default anyway. The only change is on armv7, which gains the unwind entries (about a hundred bytes per  [... too long, see https://mrelay.p2pool.observer/e/3p_cy60LekItLUhN ]