Bug 197769 - Padding for blowfish encrytion is not done correctly
Summary: Padding for blowfish encrytion is not done correctly
Status: RESOLVED FIXED
Alias: None
Product: konversation
Classification: Applications
Component: general (show other bugs)
Version: unspecified
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Travis McHenry
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-24 21:40 UTC by lubyou
Modified: 2009-06-28 20:18 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
proposed patch, note that i am not a c++ developer, so things might not confirm to your coding guidelines (2.41 KB, patch)
2009-06-24 21:43 UTC, lubyou
Details
append \0's for padding (1.99 KB, patch)
2009-06-25 00:18 UTC, Travis McHenry
Details

Note You need to log in before you can comment on or make changes to this bug.
Description lubyou 2009-06-24 21:40:41 UTC
Version:            (using KDE 4.2.90)
OS:                Linux
Installed from:    Ubuntu Packages

Blowfish requires strings to be a multiple of eight. While this is done in konversation, the padding is not done correctly. 

QByteArray::resize is used to resize the array to a length which is multiple of eight, but it is padded with random values ("(..) byte array is extended to make it size bytes with the extra bytes added to the end. The new bytes are uninitialized.")

This leads to different encrypted strings for the same plain text in ECB mode and it also makes it impossible for implementations from other clients to know where the plain text string actually ends.

Padding should be done with \0, which is what the other implementations i have looked at do.
Comment 1 lubyou 2009-06-24 21:43:14 UTC
Created attachment 34792 [details]
proposed patch, note that i am not a c++ developer, so things might not confirm to your coding guidelines
Comment 2 Travis McHenry 2009-06-24 23:28:27 UTC
Have you seen any actual problems with encryption for this? I looked into the Qt code and it adds \0 as far as I can tell. 

Please give me a reproducible situation where encryption problems arise.
Comment 3 Travis McHenry 2009-06-25 00:17:49 UTC
(discussed on irc) try this patch
Comment 4 Travis McHenry 2009-06-25 00:18:42 UTC
Created attachment 34793 [details]
append \0's for padding

Try this patch
Comment 5 Travis McHenry 2009-06-28 20:18:50 UTC
SVN commit 988801 by tjmchenry:

Implement DH1080 key exchange /keyx
Add a combobox in the settings dialog to choose CBC or ECB (disabled when qca not present)
Rework Cipher object to have one in every query and channel (needed for key exchange)
ECB is now default, it's more commonly used and should help usability.

Fix padding to add only \0 to the end and not random bytes
BUG:197769

 M  +123 -23   cipher.cpp  
 M  +12 -2     cipher.h  
 M  +118 -72   config/connectionbehavior_config.ui  
 M  +5 -0      config/konversation.kcfg  
 M  +4 -0      config/settingsdialog.cpp  
 M  +5 -8      irc/channel.cpp  
 M  +13 -1     irc/channel.h  
 M  +34 -13    irc/inputfilter.cpp  
 M  +20 -1     irc/outputfilter.cpp  
 M  +1 -0      irc/outputfilter.h  
 M  +4 -0      irc/query.cpp  
 M  +14 -0     irc/query.h  
 M  +111 -12   irc/server.cpp  
 M  +10 -0     irc/server.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=988801