Re: Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt
On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote: kwalletd/backend/kwalletbackend.cc, line 635 https://git.reviewboard.kde.org/r/115497/diff/1/?file=242022#file242022line635 Seems to be no error checking here, if this fails and we overwrite the hashed passwords on disk, couldn't there be data loss when we try to re-open them (when the user can't guess the key)? Added a runtime check to decide if we shuold swap or not the hashes, also checking it in BlowfishPersistHandler::write. This will fix the following usecase: -KWallet uses SHA1 to read -KWallet uses PBKDF2 to write, but BKDF2 hash is null For other cases we can't add much fallback since Backend::setPassword returns void, and no other code using it checks for errors in anyway. - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/#review49065 --- On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/ --- (Updated Feb. 5, 2014, 3:10 p.m.) Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu. Repository: kde-runtime Description --- Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt. I would have loved to completely replace it once the wallet is ported to the new hashing but because of kwalletd code that is not possible without a bigger rewrite. There are 2 reasons for this patch: 1-We avoid using our own implementation of SHA 2-We use a modern hashing technique I'm cooking more patches to use the system user password to open the wallet, we want that password to be hashed using PBKDF2_SHA512 for security reasons. Diffs - CMakeLists.txt 275a6c7 cmake/modules/FindLibGcrypt.cmake PRE-CREATION kwalletd/backend/CMakeLists.txt 5a5837c kwalletd/backend/backendpersisthandler.cpp bdef6ca kwalletd/backend/kwalletbackend.h 83ebf7f kwalletd/backend/kwalletbackend.cc e4d461c Diff: https://git.reviewboard.kde.org/r/115497/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt
On Feb. 5, 2014, 7:18 p.m., Michael Pyne wrote: kwalletd/backend/kwalletbackend.cc, line 130 https://git.reviewboard.kde.org/r/115497/diff/1/?file=242022#file242022line130 The salt here seems to be based off of the user's login-name, which can change (for instance, someday my KDE git user account will change away from kde-svn... I've just been lazy so far ;). Beyond that the salt should really be random bytes otherwise you could still build rainbow tables of the top-100 most popular user names, for instance. Using random bytes would complicate this since you'd have to actually store the salt and re-load it to re-derive the right key, but it's the right way to do it. In any event it's probably more important to ensure that there's no data loss if the user tries to open their wallet under a different UNIX login, even if we have to use a plain constant string as the salt. Àlex Fiestas wrote: I decided to use the username because this hash will be produced from a pam module as well, accessing users data from there is kind of messy and we will have to generate and then chown that salt so I decided to go with a shortcut for the time being. Will implement proper salt in a future patch if that's ok for you. Actually that will force me to update again the hash which is messy, so going to write that code right now. - Àlex --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/#review49065 --- On Feb. 5, 2014, 3:10 p.m., Àlex Fiestas wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/ --- (Updated Feb. 5, 2014, 3:10 p.m.) Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu. Repository: kde-runtime Description --- Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt. I would have loved to completely replace it once the wallet is ported to the new hashing but because of kwalletd code that is not possible without a bigger rewrite. There are 2 reasons for this patch: 1-We avoid using our own implementation of SHA 2-We use a modern hashing technique I'm cooking more patches to use the system user password to open the wallet, we want that password to be hashed using PBKDF2_SHA512 for security reasons. Diffs - CMakeLists.txt 275a6c7 cmake/modules/FindLibGcrypt.cmake PRE-CREATION kwalletd/backend/CMakeLists.txt 5a5837c kwalletd/backend/backendpersisthandler.cpp bdef6ca kwalletd/backend/kwalletbackend.h 83ebf7f kwalletd/backend/kwalletbackend.cc e4d461c Diff: https://git.reviewboard.kde.org/r/115497/diff/ Testing --- Thanks, Àlex Fiestas
Re: Review Request 115515: [oxygen] Check whether we are on platform X11 before calling into xcb
On Feb. 6, 2014, 12:26 p.m., Hugo Pereira Da Costa wrote: @Martin in kstyles/oxygen you are missing oxygenblurhelper (and likely kate will crash when showing a tooltip) in kwin/clients/oxygen (but might be another review) oxygenclient oxygensizegrip config/oxygendetectwidget Other than that, ship it ! kwin/clients doesn't matter (yet) ;-) - Martin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/#review49102 --- On Feb. 6, 2014, 11:55 a.m., Martin Gräßlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/ --- (Updated Feb. 6, 2014, 11:55 a.m.) Review request for kde-workspace and Hugo Pereira Da Costa. Repository: kde-workspace Description --- [oxygen] Check whether we are on platform X11 before calling into xcb Just because we compiled with X11 present doesn't mean we run on X11. This fixes quite a lot of crashers when trying to run framework apps on Wayland. @Hugo: do you know of further files which use xcb unconditionally and which I just haven't hit yet? Diffs - kstyles/oxygen/CMakeLists.txt ca9cc1a1710187f5013482ef502c456238fd4373 kstyles/oxygen/oxygenshadowhelper.cpp f77093daa4f907afd55333032c6f6b618ad2f47f kstyles/oxygen/oxygenstylehelper.cpp 7e2de41822bf0205e2cec7ea82c8e3f6751a2a6b kstyles/oxygen/oxygenwindowmanager.h 7ef54d3172b0b0a92b6c33152858cb923b9d3f1e kstyles/oxygen/oxygenwindowmanager.cpp 308ce4d049b6ce4c9c2cdd67448d351747f34b19 libs/oxygen/oxygenhelper.h 8d734e6e3f3a23c0257cc400417c340ba6d48bea libs/oxygen/oxygenhelper.cpp 1fe78c24c053e74f2179de00ee9a5701997a0acb Diff: https://git.reviewboard.kde.org/r/115515/diff/ Testing --- running Kate on Wayland till it crashes (or doesn't) Thanks, Martin Gräßlin
Re: Review Request 115515: [oxygen] Check whether we are on platform X11 before calling into xcb
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115515/ --- (Updated Feb. 6, 2014, 1:22 p.m.) Review request for kde-workspace and Hugo Pereira Da Costa. Changes --- added test to blurhelper. It hadn't crashed as createAtom was adjusted to return 0. Repository: kde-workspace Description --- [oxygen] Check whether we are on platform X11 before calling into xcb Just because we compiled with X11 present doesn't mean we run on X11. This fixes quite a lot of crashers when trying to run framework apps on Wayland. @Hugo: do you know of further files which use xcb unconditionally and which I just haven't hit yet? Diffs (updated) - kstyles/oxygen/CMakeLists.txt ca9cc1a1710187f5013482ef502c456238fd4373 kstyles/oxygen/oxygenblurhelper.cpp d774b2c45f8ec452311d34f35adf4d9bcc39693d kstyles/oxygen/oxygenshadowhelper.cpp f77093daa4f907afd55333032c6f6b618ad2f47f kstyles/oxygen/oxygenstylehelper.cpp 7e2de41822bf0205e2cec7ea82c8e3f6751a2a6b kstyles/oxygen/oxygenwindowmanager.h 7ef54d3172b0b0a92b6c33152858cb923b9d3f1e kstyles/oxygen/oxygenwindowmanager.cpp 308ce4d049b6ce4c9c2cdd67448d351747f34b19 libs/oxygen/oxygenhelper.h 8d734e6e3f3a23c0257cc400417c340ba6d48bea libs/oxygen/oxygenhelper.cpp 1fe78c24c053e74f2179de00ee9a5701997a0acb Diff: https://git.reviewboard.kde.org/r/115515/diff/ Testing --- running Kate on Wayland till it crashes (or doesn't) Thanks, Martin Gräßlin
KDE Review: Move kqtquickcharts to KDE Edu
Hi everyone, kqtquickcharts (formerly known as kqmlgraphs) provides components for line and bar charts for QtQuick applications. As discussed earlier, I want to release the project as soon as possible (read: 4.13) so KTouch and artikulate can use it. The release of artikulate depends on this, KTouch will drop its own copy of the charts code as soon as the review progress is over. Ideally I want to release the project in the current state with as little changes possible. Large new features and refactorings are reserved for the inevitable port to QtQuick 2. With that, I may try to get project into Frameworks 5, depending on interest. For now, I stick with KDE Edu. The repository for the review is located at: https://projects.kde.org/projects/kdereview/kqtquickcharts/repository Best regards, Sebastian
Re: Review Request 115497: Replace SHA with PBKDF2-SHA512+Salt
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115497/ --- (Updated Feb. 6, 2014, 3:28 p.m.) Review request for KDE Runtime, Teo Mrnjavac and Valentin Rusu. Changes --- Fixed all issues, added generated salt. Still I haven't changed it to SCRYPT until I get some more feedback. Repository: kde-runtime Description --- Uses the MINOR_VERSION (which until now it was 0) to upgrade the hash from SHA to PBKDF2-SHA512+salt. I would have loved to completely replace it once the wallet is ported to the new hashing but because of kwalletd code that is not possible without a bigger rewrite. There are 2 reasons for this patch: 1-We avoid using our own implementation of SHA 2-We use a modern hashing technique I'm cooking more patches to use the system user password to open the wallet, we want that password to be hashed using PBKDF2_SHA512 for security reasons. Diffs (updated) - CMakeLists.txt 275a6c7 cmake/modules/FindLibGcrypt.cmake PRE-CREATION kwalletd/backend/CMakeLists.txt 5a5837c kwalletd/backend/backendpersisthandler.cpp bdef6ca kwalletd/backend/kwalletbackend.h 83ebf7f kwalletd/backend/kwalletbackend.cc e4d461c Diff: https://git.reviewboard.kde.org/r/115497/diff/ Testing --- Thanks, Àlex Fiestas
Review Request 115519: Do not use KDE_VERSION_STRING for workspace applications
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/115519/ --- Review request for kde-workspace and Release Team. Repository: kde-workspace Description --- Workspace is no longer synced with kdelibs releases. Thus if compiled against e.g. 4.12 we want our workspace apps to still be 4.11. Diffs - kwin/clients/oxygen/demo/main.cpp 83d9d83 kwrited/kwrited.cpp cab2d6b plasma/desktop/shell/main.cpp ea3d43d startkde.cmake 30d5ab5 statusnotifierwatcher/statusnotifierwatcher.cpp 97ae164 systemsettings/app/main.cpp 78109e0 kstyles/oxygen/config/main.cpp 5a5286e kstyles/oxygen/demo/main.cpp 005395b ksysguard/gui/ksysguard.cpp 2ad34f2 config-workspace.h.cmake b3ba37d khotkeys/kcm_hotkeys/kcm_hotkeys.cpp 389a361 kinfocenter/main.cpp c28f7cf krunner/main.cpp 6eac316 kscreensaver/kblank_screensaver/blankscrn.cpp 99ef649 kscreensaver/krandom_screensaver/random.cpp 4047184 Diff: https://git.reviewboard.kde.org/r/115519/diff/ Testing --- compiled with 4.12.60 kdelibs and looked at e.g. startkde and systemsettings which have now 4.11.6. Thanks, Martin Gräßlin
