Open dsweber2 opened 1 year ago
nitz
commented
1 year ago Oh dear. "Generic Error" sounds super useful, doesn't it? 😂
At least it's not a NotImplentedException. I'll have to set up an environment to take a look at it, but it at least feels fixable!
dsweber2
commented
1 year ago Definitely not the most helpful error XD. Appreciate you looking into it; its generally still usable, but for verbally sharable passwords (e.g. wifi), having Latin 1 supplement by default makes them effectively unsaltable.
nitz
commented
1 year ago I know it's a pain in the butt, but as a temporary workaround, you can edit your KeePass' config.xml where the settings for the generator are saved manually.
Also, just checking, you do use the default mate wm, yeah?
dsweber2
commented
1 year ago yep! basic mate.
Do you happen to have an example of the config settings? I went looking in ~/.config/KeePass/KeePass.config.xml, /var/lib/keepass2/KeePass.config.xml, and ~/KeePass/KeePass.config.xml but there were no settings for it in any of them, even after I saved a custom settings specifically to use diceware.
nitz
commented
1 year ago Okay so, it's a bit cumbersome (because when I did the options saving, I used XML serialization so it ends up being escaped XML inside XML.)
On my windows machine it's in the KeePass.config.xml that lives beside my KeePass executable (which I'm assuming lives there just because the working directory and I run in standalone mode) so I'm not positive where yours should be, but if you exit KeePass you can be sure it will at least write the current configuration before it quits.
But in the config, it should be in the XML Path Configuration\PasswordGenerator\UserProfiles\, and you should be able to find a <Profile> element with a <Name> element who's value is set to the name you saved. In that same <Profile>, is a <CustomAlgorithmOptions> element that has a whole ass escaped XML string. (thus the double serialization I mentioned above.)
In that mess is an element (albeit escaped) named <SaltCharacterSources>, which has a space delimited value list of the source options. You should be able to remove Latin1Supplement from that list by simply deleting it!
Make sure to do this while KeePass is closed, as it doesn't read/write the config file often, and it's easy to end up overwriting it if it's running and losing edits you've made. You can also add any of the other values for sources by using their Key (which is just their name without spaces, of which only Latin1Supplement currently has.)
Let me know if that helps!
SecurityCze
commented
2 months ago Hello, I have taken a look at this issue. And for me it is still broken (atleast on Ubuntu 20.04.6 running inside WSL). The crash is caused by mono and unicode emojis.
When opening the SaltSourcesForm application attempts to load all default salt sources. Including a unicode emojis.
However mono can not handle it and crashes. Same issue (also in Keepass) was already reported to mono. But it still seems to not be fixed.
Manually remove emojis from config (untested) If you remove them, then it should be possible to open configuration and continue without problems (just remember to not reset the config).
Use Wine instead of mono
It is also supported by KeePass. However it requires a bit more setup (compered to apt install keepass2 or similar)
Remove emoticons from default sources Simplest solution, users can still add them if they want.
Runtime dependent code We can detect when the plugin is running inside mono and only then remove emotes. However it complicates the code.
I would personally vote for option number 3. It is the simplest fix without much impact. If @nitz is interested in any of the fixes - I can whip up a pull request.
nitz
commented
2 months ago @SecurityCze I overwhelmingly appreciate you digging into this and figuring out the root cause. I did a bit of a twist on your 3 and 4 options there, and was hoping you could give it a shot if you had the time. It's at 64fe7e6bc6377a4e26ef7b273fc609133238b20c
I based it on that the mono bug you linked there does seem to produce an exception rather than outright crashing, so I'm hoping by checking for any exception thrown when simply attempting to render an emoji, I can decide to not offer it by default.
This unfortunately won't handle a case where the emoji salt source is already saved in the user's config, I realize now as I'm typing this out! 😂
(see next comment)
nitz
commented
2 months ago Maybe this will fix that last point I had without too much complication. At aa663fde8a32c1bd7ac7cc490066aaf692493406, I added a test of each salt source as it will be populated in the config dialog. If it fails, it removes it from the sources put in the dialog, which will then allow the user to save that "safe" configuration.
There's still a very small edge case where someone has enabled emoji (or added any sort of character that mono's GDI can't render) via another platform or manually to the config, where I'm assuming it'd fail to render in the actual password dialogs, but I think that's out of scope for this fix, which should hopefully be enough for the vast majority of any mono on Ubuntu users.
SecurityCze
commented
2 months ago I come bearing bad news... (such a shame, it was a nice solution)
It seems that it only throws an exception when actually rendering something on screen in specific conditions.
The test https://github.com/cmdwtf/KeePassDiceware/blob/aa663fde8a32c1bd7ac7cc490066aaf692493406/SaltSources.cs#L155 passes just fine. And then, when the form is rendered, it throws GenericError.
I just found this comment.
An exception is thrown while Mono draws a list view item
text boxes don't display such characters at all
Unless something changed this crash problem is limited to ListViewItem.
And I can confirm that. When I save the emoji as an advanced string field (in some entry), the text box is rendered as empty. But if I save it as username it crashes when rendering the list.
nitz
commented
2 months ago Oh dear. What an absolutely bizarre issue to only occur in listviewitems like that.
I was gonna just say "well I guess I'll just do the platform/runtime check as duct tape for now," but then I got curious and started digging anyways.
The tl;dr: I think the issue may be coming from the custom StringFormat uses to draw the text with, but the issue doesn't show up until the next GDI+ call. It may have something to do with the size of the destination render area, but I didn't track it down that far I added a string format to the draw text call, and as well a fill rectangle call after it. If this doesn't trip it, then I think just the platform check will have to do. Here's that if you'd like to try: @ a390e6c34d886c175c918738aed6b075f6a45866
These were the notes I was taking as I was investigating:
Looking at the callstack on that bug you initially linked, the exception looks like it's coming from the FillRectangle call? I chased it down into libgdiplus, and the only spot I see there that's returning the "generic error" seen in that exception is if there's no backend:
I didn't follow the path all the way into cairo, but it is possible it's causing it in the translation from the cairo status to gdip's status, but I'm gonna just imagine that's a red herring at the moment.
Stepping a bit further back up the stack, I took a look at the ThemeWin32Classic's drawing of the list view, and it looks like the fill rectangle calls are the earliest calls to anything on the dc:
Now it certainly makes sense to me that it would draw the background rectangle before the text (duh, lol) but I can't see anything there that would make sense as to why libgdiplus would be falling over before it's even handed a string with emoji characters in it.
So at this point, I'm thinking the GDI+ state must get corrupted by a previous DrawListViewSubItem() call, since in a subsequent item the previous GDI+ call would have be to DrawString() if that item had text. As you'd expect, DrawString()'s implementation is as straightforward as possible, same with it's native call containing the same style switch that only returns GenericError for no backend:
Looking at the cairo implementation of string drawing, it seems like the code path can vary greatly depending on formatting flags. This is what gave me the idea to try adding the same string format that the DrawListViewSubItem produces to the test.
SecurityCze
commented
2 months ago Wow, nice work.
I tested it. And it is detecting the crashes correctly this time.
Unfortunately it broke indexing of the sources. When the source is removed and you then attempt to add a new source (by clicking on last/empty row) it crashes with ArgumentOutOfRange.
System.ArgumentOutOfRangeException: Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: index
at System.Collections.ArrayList.get_Item (System.Int32 index) [0x00013] in <12b418a7818c4ca0893feeaaf67f1e7f>:0
at System.Windows.Forms.DataGridViewCellCollection.GetCellInternal (System.Int32 colIndex) [0x00006] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at (wrapper remoting-invoke-with-check) System.Windows.Forms.DataGridViewCellCollection.GetCellInternal(int)
at System.Windows.Forms.DataGridView.GetCellInternal (System.Int32 colIndex, System.Int32 rowIndex) [0x0000c] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.OnCellEnter (System.Windows.Forms.DataGridViewCellEventArgs e) [0x0000d] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.SetCurrentCellAddressCore (System.Int32 columnIndex, System.Int32 rowIndex, System.Boolean setAnchorCellAddress, System.Boolean validateCurrentCell, System.Boolean throughMouseClick) [0x00237] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.MoveCurrentCell (System.Int32 x, System.Int32 y, System.Boolean select, System.Boolean isControl, System.Boolean isShift, System.Boolean scroll) [0x00135] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.OnRowsPreRemovedInternal (System.Windows.Forms.DataGridViewRowsRemovedEventArgs e) [0x0010c] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at (wrapper remoting-invoke-with-check) System.Windows.Forms.DataGridView.OnRowsPreRemovedInternal(System.Windows.Forms.DataGridViewRowsRemovedEventArgs)
at System.Windows.Forms.DataGridViewRowCollection.RemoveInternal (System.Windows.Forms.DataGridViewRow dataGridViewRow) [0x00012] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.OnUserAddedRow (System.Windows.Forms.DataGridViewRowEventArgs e) [0x0002b] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.SetCurrentCellAddressCore (System.Int32 columnIndex, System.Int32 rowIndex, System.Boolean setAnchorCellAddress, System.Boolean validateCurrentCell, System.Boolean throughMouseClick) [0x00296] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.MoveCurrentCell (System.Int32 x, System.Int32 y, System.Boolean select, System.Boolean isControl, System.Boolean isShift, System.Boolean scroll) [0x00135] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.EndEdit (System.Windows.Forms.DataGridViewDataErrorContexts context) [0x000e0] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.EndEdit () [0x00000] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.OnMouseDown (System.Windows.Forms.MouseEventArgs e) [0x00007] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.Control.WmLButtonDown (System.Windows.Forms.Message& m) [0x00097] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.Control.WndProc (System.Windows.Forms.Message& m) [0x00177] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.DataGridView.WndProc (System.Windows.Forms.Message& m) [0x00000] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.Control+ControlWindowTarget.OnMessage (System.Windows.Forms.Message& m) [0x00000] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.Control+ControlNativeWindow.WndProc (System.Windows.Forms.Message& m) [0x0000b] in <a3daa9b84fd241a497578a25f68bc3c7>:0
at System.Windows.Forms.NativeWindow.WndProc (System.IntPtr hWnd, System.Windows.Forms.Msg msg, System.IntPtr wParam, System.IntPtr lParam) [0x0008e] in <a3daa9b84fd241a497578a25f68bc3c7>:0I did not have time to dig deeper (and unfortunately I did not manage to get remote debugging in WSL working). Maybe later in the week I can find a bit of time to find and fix it.
And there is also an (unrelated?) bug (even in master branch) that the application crashes when removing a row of salt. It also throws an OutOfRangeException so it might be the same issue? https://github.com/cmdwtf/KeePassDiceware/blob/a390e6c34d886c175c918738aed6b075f6a45866/SaltSourcesForm.cs#L163
So it might need a bit of rework of how validation and adding/removing of salts is handled. That might fix both issues in one.
SecurityCze
commented
2 months ago All right, I have taken one more look at the crashes on Mono. And I think we encountered another Mono issue :/.
An exception is thrown inside Mono when adding a new row. Usually on windows, when clicked on empty row, default values are prefilled (and that probably means that a new object is created).
However on mono, when clicking on empty row, nothing is prefilled. And when you attempt to write to a field it cannot modify an object that it did not yet create.
But it does not end here. It only crashes when creating a first new row (for each instance of the form). When you click the empty line and then click away, the correct object is created.
After which you can modify it no problem.
What is even stranger is that any additional attempts to create a new line (I clicked on the empty row and set the name to "a") cause TWO additional "New source" rows to appear.
If you just click on the empty line, and immediately click away, the two "New source" rows are still created.
If this is not enough - the row validation is not working on Mono. There appears to be some strange delay where it is considering a state few changes behind.
I even reworked SaltSourcesForm (in commit f3f7eb56492c349db183a3f68faa5fc32fddc8d5) to use data bound DataGridView in hopes that it handles it better. But inserting is still broken the same way. And validation is, for a change, not working at all.
As it stands now I do not know how to fix this. The exception (as you can see in the stack trace) is not coming from our code. So I do not even know where we could catch this and implement some workaround.
What might help is attaching a debuger when running on Mono. So you can get mode visibility into what is going on. However I was not able to get this to work.
So unless anyone has any ideas: I am personally happy with what fixes you did. At least it crashes in only one situation.
I would only recommend modifying the warning message with the right ?pronoun? (not sure if that is the correct term for "they are"). Currently even if one salt source was removed it says that "1 salt source was removed because they are not supported..." - it should read it is.
Glad that this exists, thanks! Kind of a strange error; when I clicked "edit salt" (to set it to get around special character requirements without using the latin 1 supplement), the program crashed. I got an error report the second time:
Maybe something about the way you're constructing that window is relying on a windows environment? I'm on Ubuntu-Mate 22.04 specifically.