Bug 51819 - Password-protected documents saved unencrypted for auto-recovery
Summary: Password-protected documents saved unencrypted for auto-recovery
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
3.4.2 release
Hardware: All All
: highest critical
Assignee: Not Assigned
URL:
Whiteboard: target:4.3.0 target:4.2.3 target:4.1.6
Keywords: regression, security
Depends on:
Blocks: mab4.1
  Show dependency treegraph
 
Reported: 2012-07-07 00:20 UTC by Florian Reisinger
Modified: 2014-03-25 10:10 UTC (History)
10 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Reisinger 2012-07-07 00:20:02 UTC
Via IRC by _nedR

When Ubuntu crashes and has to be restarted via commandline
After restarting the PC and starting LibreOffice and, of cource, recovering the file, the password protecion got lost....
Comment 1 Nidal R 2012-07-07 09:55:20 UTC
I managed to reproduce this on Ubuntu 12.04 (LibreOffice v3.5.3) by doing the following steps:

1. Save and close whatever documents you may have open in LibreOffice
2. Open a new document in LibreOffice (Writer, Calc, Draw, or Impress); Enter some data into it.
3. Save the document with a password in any file-format.
4. Go to Tools>Options; Select the Load/Save>General page of options; Ensure the entry "Save AutoRecovery information every" is checked and set the "Minutes" value to 1 minute and press OK.
5. Enter some more data.
6. After waiting for at least 1 minute for the AutoRecovery info to be saved, open up a terminal and kill soffice.bin
7. Open LibreOffice

Expected behaviour :- LibreOffice Should display the File AutoRecovery Dialog box. On clicking Next; It should prompt for a password before recovering the document.

Actual behaviour :- LibreOffice successfully recovers the file without asking for the password. Furthermore when you save the file, it is saved without a password.

I tried to reproduce this error on my Windows partition which had LibreOffice v 3.4.1 but couldn't, so I imagine this is a recent regression.

I also wonder whether this implies that the AutoRecovery file is stored unencrypted on the filesystem, because that would be less than ideal from a security point of view.
Comment 2 Cor Nouws 2012-07-07 14:50:31 UTC
Indeed I can reproduce it in the way described...
And I see the problem in 3.5.3 too. And in 3.4.6.
But not in 3.3.4 ...
So it has to do with the major encryption change in that time?

(set version to 3.4.6 release
have checkd 3.4.0, that does not have the problem!
but I did not check versions 3.4.1-3.4.5 .. )
Comment 3 Cor Nouws 2012-07-07 15:24:18 UTC
hmm, it does happen for the first time in 3.4.6 !
Comment 4 mikef108 2013-02-28 00:00:04 UTC
This bug is happening to me in Windows XP Home SP3 in LO 4.0.0.3 (Build ID: 7545bee9c2a0782548772a21bc84a9dcc583b89).

After a crash, or a power failure, all of the open files lose their passwords. The files that weren't open at the time of the crash don't lose their passwords.

It might have something to do with the  Save with password  checkbox in the Save As dialog box becoming unticked. Perhaps the Recovery function doesn't recover the password.

This is a major problem to me because the info in the files really needs to be password protected.

I think that LO is better than OpenOffice, but I'll be using OO until this problem is fixed.
Comment 5 sayt 2013-03-26 09:23:32 UTC
Confirmed here: Windows XP, LibreOffice 3.6.5.2

Unfortunately also confirmed that the recovery file is saved WITHOUT ANY protection, fully unencrypted into the backup directory (user/backup)!

IMHO it is a very nasty bug in terms of security, that should seriously be taken into account, and resolved as soon as possible!

Now by switched on recovery the "password protected" document gets saved every 5-10 minutes without ANY protection, easily recoverable by not much advanced techniques (eg. undeleting the backup file). It is a MAJOR problem in terms of security, and users definitely should be warned of this, making them switch off the auto-recovery as a temporary workaround.
Comment 6 sayt 2013-03-26 09:36:29 UTC
Changed title to stress importance.
Comment 7 Florian Reisinger 2013-03-26 09:45:48 UTC
Nominating as HardHack...
Comment 8 mikef108 2013-05-13 00:37:02 UTC
As well as the password being lost in LO, OO 3.4.1 Build 9593 on Windows XP Home SP3 loses it as well. The computer crashed and blue-screened. When the document was re-opened, the password protection had disappeared. 

My previous post:
> This bug is happening to me in Windows XP Home SP3 in LO 4.0.0.3 (Build ID: 7545bee9c2a0782548772a21bc84a9dcc583b89).

> After a crash, or a power failure, all of the open files lose their passwords. The files that weren't open at the time of the crash don't lose their passwords.

> It might have something to do with the  Save with password  checkbox in the Save As dialog box becoming unticked. Perhaps the Recovery function doesn't recover the password.

> This is a major problem to me because the info in the files really needs to be password protected.


Now I'm thinking of using OxygenOffice instead. But I bet it has the same bug because OxygenOffice is built from OpenOffice!
Comment 9 Florian Reisinger 2013-05-13 13:08:55 UTC
I really ask you to be patient...
Comment 10 Joel Madero 2013-05-14 04:22:54 UTC
This issue is being raised - thanks Florian for bringing it to my attention - I will be letting the appropriate people know about it
Comment 11 Nidal R 2013-05-14 05:05:06 UTC
It's great to hear that this issue is finally receiving the attention it deserves. Thanks, guys!
Comment 12 Michael Meeks 2013-05-22 10:23:59 UTC
Patches / volunteer developer input most welcome - of course.
Comment 13 Julien Nabet 2013-10-19 21:02:45 UTC
On pc Debian x86-64 with master sources updated today, I don't reproduce this.
Now, perhaps I missed something since autorecovery didn't even appear (although I had run a kill -9)
Comment 14 Björn Michaelsen 2014-01-17 00:43:48 UTC
(This is an automated message.)

LibreOffice development currently prioritizes bugs with the so called MAB (most annoying bugs) -- as this bug has not run through that process (including writing a short rationale for this bug being a candidate and other who are watching the tracker bug silently approving that rationale etc.) its priority is set to high. Note this is effectively no change in the urgency assigned to this bug, as we are currently not making a difference between high and highest and severity is untouched.

You can find out more about MABs and how the process works by contacting libreoffice qa on irc:

 http://webchat.freenode.net/?channels=libreoffice-qa

The QA wiki page also gives you hints on how to get in contact with the team (if IRC fails you, your next best choice is the mailing list):

 https://wiki.documentfoundation.org/QA
Comment 15 sayt 2014-01-21 08:07:13 UTC
Any update on this, is anybody working on it?

Should not be a critical security bug taken more into account, or at least the users warned?

And why got the hard hack stuck in the proposed state, should not it be accepted to start the work?

Is this bug maybe an NSA "feature"? (Just kidding... hopefully)
Comment 16 sayt 2014-01-25 13:00:39 UTC
Just tried, still present in 4.1.4.2

Sorry to say this, but if there is no progress, I will try to inform the users and the media directly about this.

It is a serious security bug, introduced in 3.4.6 (!), completely ignored for more than 1,5 year (!!) now.

The users have the right to know about this security issue, even if it could potentially cast some negative light on LO.
Comment 17 Michael Meeks 2014-01-25 18:50:12 UTC
sayt@mailinator.com - thanks for your feedback; security is clearly important to you - if you're deeply concerned about your data in this way, I would strongly suggest using an encrypted disk: that way document level security is less important, and your data safer overall.

If you want to turn your passion into something constructive here, you are more than welcome to work with us to study the code, provide a patch and address the issue: as a community we're eager to help get new people involved, with code pointers, build instructions etc. The ability to type, think and dedicate time to improving the code for others are the only pre-requisites to starting the journey to be an elite coder: you can do it !

In the absence of that your suggested approach is unlikely to win many friends in the community you care about; no doubt the underlying issue will be fixed one day - in the medium term I want to get rid of autosave altogether and stream unsaved edits to the file in question as/when we do that encryption will no doubt be part of the picture.

All the best.
Comment 18 Cor Nouws 2014-01-25 20:07:57 UTC
(In reply to comment #16)

> The users have the right to know about this security issue, even if it could
> potentially cast some negative light on LO.

To be honest I never look at the wording in the dialog on document recovery.
Could it be that there is space there to inform users to check password protected files that were active when a crash occured?
Comment 19 sayt 2014-01-26 16:45:56 UTC
@Michael Meeks - thanks for your answer, but I think you clearly not understood the severe of this bug.
To clear some point:
1. Security is not important only for me, it is important for everybody who uses password-protection.
2. _I_ personally know how to circumvent this bug, but a lot of people does not even dream of having such a big security hole in LO (for more than 1 year), so they are totally helpless against it.
3. You advertised the encryption changes in 3.5 as "more secure", while it is just the opposite, saving now a password-protected document every 10-15 minutes completely unencrypted all over the file system.
4. I am pretty sure, that it would take only a 30-45 minute debugging session for an experienced developer to find the hole and fix it (maybe just some false flag set somewhere); and I am also almost sure, that nobody took the time to do it from the dev team in the past 1,5 year.
5. I tried to download and compile the sources last year to fix this issue, but it was anything else than easy or self-explaining. Maybe I will try again now, but cannot promise anything.
6. If somebody without any dev. experience finds in LO a severe security issue, than he has to fix it himself, or nothing happens? And even if he did it, would you accept a security patch from anybody out there, outside of the core team? Sorry, but it sounds a bit lame... (NSA sends its greetings)
7. Since 3.5 a lot of minor changes / improvements were made, so there were clearly no resource problem to fix such an issue.
8. You did not even accept the proposed hard hack (a critical security bug!) without even explaining why.
9. The users DO have the right to know about this issue, and I am pretty sure that if they did, this bug would get a lot more noise and complaining.
10. I am just a regular user not a developer, but "to turn my passion into something constructive here" I suggest you to take a look around bug 40006 and bug 43868 and maybe in a short time this bug could be fixed at last without further headaches.
Comment 20 Julien Nabet 2014-01-26 18:11:11 UTC
sayt@mailinator.com: about building LO you can find great help here:
https://wiki.documentfoundation.org/Development
Comment 21 Joel Madero 2014-01-26 18:31:53 UTC
I'm not going to comment too much more on this but let me just correct one thing. Fixing this will likely take days or weeks of dev time not the 30-45 minutes that was stated. If you're not a develop working on our code you can't possibly know how much time something takes to fix so throwing out numbers is just another good way to not make friends in the community.

Please feel free to build LibreOffice as Julien has provided a link and we have an IRC chat where we can also help. I have almost no CS background and I build based on those instructions.

Also if you're running Linux these are good instructions: https://wiki.documentfoundation.org/Development/BuildingOnLinux
Comment 22 Nidal R 2014-01-27 08:45:58 UTC
"There are two kinds of cryptography in this world: cryptography that will stop your kid sister from reading your files, and cryptography that will stop major governments from reading your files."

This bug although applicable in case 2 also (for which you can workaround by encrypting your file system), has more to do with case 1. 
eg:- Your system crashes (say power failure). Someone else (say your spouse)  uses your computer when the power comes back; opens LibreOffice, gets prompt to recover files. opens document you didn't want them to see (say divorce filing you have been drafting). This bug is very nasty for that reason - Its well-hidden, and manifests itself in a very bad way.

This bug is a regresssion since version 3.4.1. My question is -has the architecture changed so much that it is hard for a developer to issue a fix? Asking someone else who is not an expert coder, to issue a fix is unrealistic. I imagine it would take at least a month of effort, for a normal developer, just to familiarize himself with the architecture of a large s/w system like LibreOffice.
Comment 23 Joel Madero 2014-01-27 14:50:28 UTC
Yes since that release a lot of lines have changed. Even for experts it takes some time to track down what is going on (millions of lines of code, developers can only be familiar with so much of it, and once they move to a new section, they have to re-familiarize themselves if they have to go back).
Comment 24 sayt 2014-01-27 16:17:26 UTC
Thanks for the links, but I think, you did not get the picture.

This bug is a problem not for me, but for LO.

A professional system cannot afford such security holes, especially not for such a long time.

Just imagine some headlines:
"Everybody can open your 'protected' LibreOffice documents without knowing the password"
"LibreOffice developers waiting for a newbie to fix the issue at last"

It is a bit absurd, isn't it?

I filed a lot of bugs into different systems, but nobody ever wanted to make me, a user responsible for fixing the bug (especially not a security one).

I think, the people who were participated in the 3.5 changes should take a look into it, solve the issue ASAP and then this bug should be buried silently as deep as possible (it is not really a good ad for LO, which I personally like very much).

I will not post here again (completely useless), but I will inform the other users about this issue if there is no progress this week.
Comment 25 Joel Madero 2014-01-27 16:34:53 UTC
and once again - these threats are meaningless. FLOSS is a community - we've done what we can to get you to actually help solve the problem and you have refused to offer anything more than threats and comments. If you ever want to actually help the project, feel free to email me personally as I have more than 10x what I can possibly handle on my plate and would love for someone (you) to volunteer to deal with some of them
Comment 26 Florian Reisinger 2014-01-28 07:38:09 UTC
(In reply to comment #24)
> Just imagine some headlines:
> "Everybody can open your 'protected' LibreOffice documents without knowing
> the password"
> "LibreOffice developers waiting for a newbie to fix the issue at last"
> 
> It is a bit absurd, isn't it?

Okay, although this might sound impolite: Repo steps
1) Start LibeOffice
2) Open an encrypted file
3) Enter password
4) Crash OS
5) Log into the same user account ([a] with no password [b] with the text on a note right in front of the PC)
> 
> I filed a lot of bugs into different systems, but nobody ever wanted to make
> me, a user responsible for fixing the bug (especially not a security one).

Let me phrase this way. The document has to be encrypted in RAM, as well as the password. IF someone really loves to, it will ever be possible...

> I think, the people who were participated in the 3.5 changes should take a
> look into it, solve the issue ASAP and then this bug should be buried
> silently as deep as possible (it is not really a good ad for LO, which I
> personally like very much).

The bad thing about this bug is, that LibO forgets, that it is password protected....

> I will not post here again (completely useless), but I will inform the other
> users about this issue if there is no progress this week.

You did understand, that without YOU (or someone else) paying for this fix it might take weeks, without months???
What has to be changed:
1) Save recovery file with password ("", if not specified)
2) After crash, try to open it with "" pw (ask for password otherwise) [for every opened document]

Quoting chromium source: "Be wary of surveillance by secret agents or people standing behind you."
Anyway, any code pointers. I found these:
http://opengrok.libreoffice.org/xref/core/oox/source/crypto/DocumentEncryption.cxx
http://opengrok.libreoffice.org/xref/core/solenv/bin/modules/pre2par/files.pm
http://opengrok.libreoffice.org/xref/core/framework/qa/complex/framework/recovery/RecoveryTools.java

I am not a dev, neither do I have spare time to help more.... (Yes, we are doing this for free)
Comment 27 sayt 2014-01-28 10:23:37 UTC
Maybe this is the routine:
http://opengrok.libreoffice.org/xref/core/framework/source/services/autorecovery.cxx#implts_saveOneDoc

It says that "if the document was loaded with a password, it should be stored with password" and also seems to set the password, but something not OK with it.
Maybe a quick debugging would show what is really missing.
Comment 28 Björn Michaelsen 2014-01-29 15:15:43 UTC
removing regression keyword as nothing in the comments suggest it is one.
Comment 29 Joel Madero 2014-01-29 15:24:35 UTC
Bjoern - haven't verified but comment 22 says something about a version working
Comment 30 Michael Meeks 2014-01-29 15:56:43 UTC
There are also a number of differences in the code paths used between save and autosave that create a number of problems; not least with image and macro loss/corruption in some circumstances that have been there ~forever and need unwinding =)
Comment 31 Alex Thurgood 2014-01-29 16:20:29 UTC
Confirming regression.

Initial test with LO 3.3.4 Mac OSX
Create Writer document
Type some text
Save with password
Set preferences to autosave every 1 minute
Keep typing more text
After 1.5 minutes, force kill LO 334
Restart LO 334, autorecovery proposes to recover document and asks for password.
Save and close document and LO.

Start LO 3.5.7
Load file.
Enter password
Check autorecovery settings to same as for LO 334
Enter some more text, wait 1.5 min
Force kill app
Restart LO 357, autorecovery proposes to recover document but does not ask for password. Document recovered without password.
Upon new save after further data entry, no password is requested.
Save, close document and LO.

Regression.
Comment 32 Alex Thurgood 2014-01-29 16:28:31 UTC
(In reply to comment #29)
> Bjoern - haven't verified but comment 22 says something about a version
> working

As does Comment #2, where Cor had already confirmed that it worked in 3.3.4 and not 3.4.6.
Comment 33 Joel Madero 2014-01-29 16:33:05 UTC
@sayt@mailinator.com - one really simple thing you can do to help us narrow down is figure out exactly what version the bug came into the code.

To do this:

http://downloadarchive.documentfoundation.org/libreoffice/old/

Just go through and download/install/test. Let us know at what point it breaks. Apparently it is between 3.3.4 & 3.4.6 (test pre-releases and releases). This way we can at least get a smaller chunk of code to investigate
Comment 34 Björn Michaelsen 2014-01-29 17:19:09 UTC
(sorry aboyt removing the regression keyword -- I only checked comments up to the point in time when it was added, no later comments).
Comment 35 sayt 2014-01-30 09:01:05 UTC
(In reply to comment #33)
OK, so the results are:

Version  Branch  Date        Bug
3.3.4.1  3.3     2011-08-04  -
3.4.0.1  3.4     2011-05-19  -
3.4.1.3  3.4     2011-06-27  -
3.4.2.3  3.4     2011-07-26  X
3.4.3.2  3.4     2011-08-25  X

So it seems to slip in somewhere between 3.4.1 and 3.4.2 (and not 3.4.6)

(If I can help more just let me know)
Comment 36 Florian Reisinger 2014-01-30 10:03:04 UTC
Here you can get even closer (via FTP) HTTP Link does not work: ftp://ftp.uni-muenster.de/pub/software/LibreOffice/testing/

Try to download a version, which is quite in the middle of the range you want to know of....
3.4.1.3-3.4.2.3

I would start with 3.4.2-rc1, because there are the most # of changes...
Comment 37 sayt 2014-01-30 12:28:19 UTC
(In reply to comment #36)
OK, the updated results:

Version  Date        Bug?
3.4.1.3  2011-06-27  -
3.4.2.1  2011-07-14  X
3.4.2.2  2011-07-21  X
3.4.2.3  2011-07-26  X

So it was already in 3.4.2.1
Comment 38 sayt 2014-01-30 12:58:57 UTC
Have found this commit in this range:

http://cgit.freedesktop.org/libreoffice/core/commit/?id=dd5f9610f5df4d6ac8062d7b94db0353f8c0fb72

Maybe some significance?
Comment 39 Joel Madero 2014-01-30 14:46:32 UTC
Thanks for investing some time into helping us narrow down when and possibly how this bug came about. I've pinged Markus about it to see what he thinks
Comment 40 Björn Michaelsen 2014-02-06 01:23:58 UTC
possibly related: fdo#37825/fdo#38561.
Comment 41 sayt 2014-02-06 14:50:55 UTC
Some more info:

AOO had the same bug as bug 37825 (or bug 38561):
https://issues.apache.org/ooo/show_bug.cgi?id=119366

Its patch was then cherry picked into LibO:
http://cgit.freedesktop.org/libreoffice/core/commit/?id=cdfad2dbbf180d3c556964c7aa8e0bb3b299d5e3

And now AOO has also the same bug as this one:
https://issues.apache.org/ooo/show_bug.cgi?id=122322
Comment 42 sayt 2014-02-06 14:52:29 UTC
Just an idea:

if PROP_ENCRYPTIONDATA would also be put into lNewArgs in this routine (as from the original document it will be cleared later in preDoSaveAs_Impl):

  http://opengrok.libreoffice.org/xref/core/framework/source/services/autorecovery.cxx#implts_saveOneDoc

... it would maybe solve the problem?

(unfortunately cannot try myself...)
Comment 43 sayt 2014-02-14 08:41:19 UTC
A quick recap on the foregoing (for future reference):

* we know now, what made this security issue appear: see comment 38, comment 41

* we have a (possible) solution to fix it: see comment 42

* the necessary changes seem to be fairly simple: 3-4 new lines to add into one file, mostly copy&paste (see comment 42)

* we are waiting for a developer who has some free hours to actually make the changes, test it and (hopefully) commit it

So it seems that we users cannot help more to get this issue solved at long last (if yes, let us know).
Comment 44 VolkerTwer 2014-02-18 18:06:53 UTC
Just stubled in, here.
My system isn't that fast so test-compiling would last quite long ...
But if one of the maintainers could check out if maybe this could work - modified lines according to mentioned
http://opengrok.libreoffice.org/xref/core/framework/source/services/autorecovery.cxx#implts_saveOneDoc 

   3233     // if the document was loaded with a password, it should be
   3234     // stored with password
   3235     utl::MediaDescriptor lNewArgs;
   3236     OUString sPassword = lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_PASSWORD(), OUString());
   3237     if (!sPassword.isEmpty())
-  3238         lNewArgs[utl::MediaDescriptor::PROP_PASSWORD()] <<= sPassword;
-  3239 


+  3238     {
+  3239         lNewArgs[utl::MediaDescriptor::PROP_PASSWORD()] <<= sPassword;
+  3240         OUString sEncryptionData = lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_ENCRYPTIONDATA(), OUString());
+  3241         lNewArgs[utl::MediaDescriptor::PROP_ENCRYPTIONDATA()] <<= sEncryptionData;
+  3242     }
+  3243
Comment 45 stragu 2014-03-04 01:48:42 UTC
I added this to 4.1 MAB.

Has anyone tested this with the 4.2 branch? I assume nothing changed but it would be good to know for sure.
Comment 46 Ryan Dunlop 2014-03-04 18:17:26 UTC
So I tried to reproduce this in (In reply to comment #45)
> I added this to 4.1 MAB.
> 
> Has anyone tested this with the 4.2 branch? I assume nothing changed but it
> would be good to know for sure.

I tried to reproduce this in 4.1.4.2.

Created document, typed, saved with password.  Typed more into document and crashed LO.  Re-opened the document and started the recovery.  Was immediately asked for password.  I will continue to "stress test" this, but it doesn't seem to be present in this build.
Comment 47 Matúš Kukan 2014-03-05 07:10:47 UTC
(In reply to comment #44)
> Just stubled in, here.
> My system isn't that fast so test-compiling would last quite long ...
> But if one of the maintainers could check out if maybe this could work -
> modified lines according to mentioned
> http://opengrok.libreoffice.org/xref/core/framework/source/services/
> autorecovery.cxx#implts_saveOneDoc 
> 
>    3233     // if the document was loaded with a password, it should be
>    3234     // stored with password
>    3235     utl::MediaDescriptor lNewArgs;
>    3236     OUString sPassword =
> lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_PASSWORD(),
> OUString());
>    3237     if (!sPassword.isEmpty())

The problem here is that sPassword is empty.
So, no, it does not work.
One would need to investigate why it's empty.
Comment 48 Markus Mohrhard 2014-03-05 07:44:51 UTC
(In reply to comment #47)
> (In reply to comment #44)
> > Just stubled in, here.
> > My system isn't that fast so test-compiling would last quite long ...
> > But if one of the maintainers could check out if maybe this could work -
> > modified lines according to mentioned
> > http://opengrok.libreoffice.org/xref/core/framework/source/services/
> > autorecovery.cxx#implts_saveOneDoc 
> > 
> >    3233     // if the document was loaded with a password, it should be
> >    3234     // stored with password
> >    3235     utl::MediaDescriptor lNewArgs;
> >    3236     OUString sPassword =
> > lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_PASSWORD(),
> > OUString());
> >    3237     if (!sPassword.isEmpty())
> 
> The problem here is that sPassword is empty.
> So, no, it does not work.
> One would need to investigate why it's empty.

SID_PASSWORD, PROP_PASSWORD are deprecated and don't work in most cases. That there are still a few users out there is unfortunate and this bug was introduced with the m106.

This code needs to be converted to use SID_ENCRYPTIONDATA which is used all over the place now and is the one making it possible to use the new encryption algorithms.

However debugging how to fix this special issue would take a while. I did it in the past for 3.4 and 3.5 to fix most of the m106 regressions but don't have time right now to look into this.

git grep SID_ENCRYPTIONDATA should give enough code pointers that show how this should be done and after that it is a good bit of debugging to follow the property though the different XPropertySets
Comment 49 mike.hall 2014-03-05 10:53:17 UTC
Security issues need special handling and absolute priority. Having bugs like this existing for more than one sub version is poor practice. Surely they should be blockers for the next version? Having one active like this one for more than 18 months is IMHO absolutely unacceptable. Do I need to escalate to one of the discussion lists? It seems crucial for an acceptable product that users can be confident that security issues are being handled quickly and with the highest priority.
Comment 50 Michael Meeks 2014-03-05 13:01:09 UTC
Hi Mike,

> Security issues need special handling and absolute priority.

Genuine security issues do; rest assured. However - volunteering other people's priority and focus is not something you can do. Since the issue is particularly important to you - can you expand on what you're doing to solve this (apparently unacceptable to you) situation ? escalation is unlikely to solve the problem - it has already been assessed by the security team & determined insufficiently serious.

As/when there is a patch, we can perhaps move here.

> It seems crucial for an acceptable product that users can be confident that 
> security issues are being handled quickly and with the highest priority.

User should be confident that we take security issues seriously. However, the level of shrillness and noise around this issue has (at least for me) radically reduced my interest in doing any work to tackle it: it is not acceptable to harass developers. Having said that, very happy to mentor anyone wanting to contribute constructively here. I hope that re-assures you.

All the best.
Comment 51 sayt 2014-03-06 10:13:24 UTC
@VolkerTwer @Matúš Kukan @Markus Mohrhard
Thanks for taking time for this issue!
It seems we are only a small step away from a working patch!
Unfortunately I cannot create one, but I think sg like this would be necessary:

Inside http://opengrok.libreoffice.org/xref/core/framework/source/services/autorecovery.cxx#implts_saveOneDoc

OUString sPassword = lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_PASSWORD(), OUString());
if (!sPassword.isEmpty())
    lNewArgs[utl::MediaDescriptor::PROP_PASSWORD()] <<= sPassword;

+ css::uno::Sequence< css::beans::NamedValue > aEncryptionData = getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_ENCRYPTIONDATA(), css::uno::Sequence< css::beans::NamedValue >() );
+ if (aEncryptionData.getLength() > 0)
+     lNewArgs[utl::MediaDescriptor::PROP_ENCRYPTIONDATA()] <<= aEncryptionData;

So PROP_ENCRYPTIONDATA would be put into lNewArgs too, when needed.

IMHO this issue in LibO was introduced this way:
1. m106 was merged, and LibO started to use PROP_ENCRYPTIONDATA.
   At that time the recovery was working without problems, but bug 37825 was introduced.
2. Bug 37825 was fixed with these commits:
http://cgit.freedesktop.org/libreoffice/core/commit/?id=dd5f9610f5df4d6ac8062d7b94db0353f8c0fb72
http://cgit.freedesktop.org/libreoffice/core/commit/?id=cdfad2dbbf180d3c556964c7aa8e0bb3b299d5e3

(BTW SID_ENCRYPTIONDATA is cleared twice now due to the two different commits)

The first commit introduced this issue, as now ENCRYPTIONDATA is cleared in PreDoSaveAs_Impl from the original document, and it is not put into the property set inside implts_saveOneDoc.

So I think the solution could be to put this into the set too.

@chtfn @Ryan Dunlop
Hi, I just tried these versions, all of them seem to have the bug too (so it is in the 4.2 branch too):

Version   Date        Bug?
4.1.4.2   2013-12-12  X
4.1.5.3   2014-02-07  X
4.2.0.4   2014-01-29  X
4.2.1.1   2014-02-14  X
4.2.2.1   2014-02-28  X

(The backup file was always unencrypted and for the recovering no password was needed.)

@mike.hall
Hi, I already tried to inform the security team about this issue (although maybe not with the best methods), interestingly they did not find it important enough (perhaps Michael pulled some strings there too?), so it seems, that we must try to solve this issue on our own somehow...

@Michael Meeks
Sorry, but I would be really eager to know what _you_ personally did to solve this issue since you took notice of it (besides maybe playing down its importance, effectively blocking it inside the dev team and forcing your own personal lower standards of security on the whole user base)?
Comment 52 Michael Meeks 2014-03-06 12:15:01 UTC
Hi Sayt,

   So - first, I am encouraged by your attempt to provide a patch and to discuss the code: that is to be applauded - if there is a real patch to test, and people are actively engaging with the code trying to contribute and understand what is wrong and help out that is -awesome- and I appreciate it. I'm personally interested in helping mentor such people, there is no shortage quite simple problems that just need a bit of research to fix in LibreOffice, and it'd be great to have help with them.

> interestingly they did not find it important enough (perhaps Michael
> pulled some strings there too?)

Ah - so perhaps I was the only one that bothered to reply to you =)

> @Michael Meeks
> Sorry, but I would be really eager to know what _you_ personally did to
> solve this issue since you took notice of it (besides maybe playing down
> its importance, effectively blocking it inside the dev team and forcing
> your own personal lower standards of security on the whole user base)?

I reproduce those in order not to comment on them :-) I basically like your challenge; it is a reflection of mine - if you want something done, -you- need to do it =) Ask not what LibreOffice can do for you etc. ... In this case I'm privileged to have Matus work for me, and (as I was encouraged by Volker's patch) I asked him to take a look at / review / test it.

In terms of personal contribution here, my hope is to provide some context, and de-escalation =) All the best.
Comment 53 sayt 2014-03-06 14:00:14 UTC
Great to hear, that Matúš is working on it "officially"! (really did not get it until now)

If I can help in anything (testing, etc.) just let me know! (except compiling for which sadly my current machine is way too old - maybe after getting a newer one somehow...)

BTW I find your strategy really great (and practical) to try to get the users involved into the community and the developing process through the issues important for them.

Just a small remark: security issues may not be the ideal candidates for this purpose, maybe they need a bit more special handling (quicker response, more focus from experienced developers, etc.)

But I personally really enjoyed browsing the code through the web interface and trying to spot the problem, and surely will try to do this for other issues too, so the strategy definitely functions :-)

Thanks for working on this issue! (and sorry for the "shrillness")
Comment 54 Joel Madero 2014-03-06 15:38:33 UTC
I think you're missing the point that our developers are almost all volunteers - so telling them what to do is impossible and disrespectful of their time. This really isn't the security concern you've made it seem to be. You tried going to the user list and multiple users (some of our experienced users) commented and none agreed with you. Furthermore, we pointed out that the real solution is encrypting a hard drive - we can't be responsible for users not knowing how to really secure their data. Lastly - commenting over and over again on the bug just makes the bug less useful as it just clogs the bug with comments that don't really say much other than "this is important TO ME" - and like I said - you tried getting other users to agree and none did on the list (there are thousands on that list by the way).

To say the least this wasn't the ideal way to push this bug - if a bug is a high priority for you - pay for a fix, or learn how to code. Else, asking us to develop processes to force volunteers to work on a bug is very unlikely to happen.

Rest assured for real security bugs you likely would have gotten more positive response from the community (users, devs, QA, etc . . .). If you actually want to contribute and become one of the thousands of volunteers putting in hours of time for free - get in touch, I have plenty of non code related tasks that need done and you can see how it can become irksome when users push for you to dedicate your time in ways that you don't find to be the most productive use of time
Comment 55 Markus Mohrhard 2014-03-06 16:46:20 UTC
(In reply to comment #53)
> Great to hear, that Matúš is working on it "officially"! (really did not get
> it until now)
> 
> If I can help in anything (testing, etc.) just let me know! (except
> compiling for which sadly my current machine is way too old - maybe after
> getting a newer one somehow...)
> 
> BTW I find your strategy really great (and practical) to try to get the
> users involved into the community and the developing process through the
> issues important for them.
> 
> Just a small remark: security issues may not be the ideal candidates for
> this purpose, maybe they need a bit more special handling (quicker response,
> more focus from experienced developers, etc.)

Just to clarify something here. Let us not call this issue a security issue. It is maybe a privacy issue if you want to give this bug a special name. Accusing us of not caring or not caring enough about security issues is not winning much friends. As Michael already mentioned earlier it has been analyzed by the security team in the past and been classified as normal bug or if you want to call it like that, "privacy" issue.

I hope that it makes a bit clearer why this bug has not seen the attention that you would have preferred. There will always be bugs where users and developers have different expectations but it would be nice to not accuse us of not caring enough.

> 
> But I personally really enjoyed browsing the code through the web interface
> and trying to spot the problem, and surely will try to do this for other
> issues too, so the strategy definitely functions :-)
> 
> Thanks for working on this issue! (and sorry for the "shrillness")

Thanks a lot for working on a fix. As you hopefully see it is much more productive to look at an issue yourself than trying to force other people to fix your bugs.

As you might have seen if you participate in fixing a bug we'll assist you bug just asking other people to fix it does not work.

Your patch looks correct if it works. If it still does not work you would need to debug it a bit and see where we either expect a SID_PASSWORD or somehow still clear the SID_ENCRYPTIONDATA.

If you are intersted in these kind of tasks you can try to remove the remaining SID_PASSWORD which might hide a few more similar problems.
Comment 56 sayt 2014-03-06 16:56:09 UTC
@Joel Madero
Thanks, got the lesson, Mr. QA!

Just please take a look on this issue, what you (QA) and what we (users) did to get this issue solved.

QA: suggesting to download and install the old versions (which in fact was a good idea), but not really more, not even accepted the Hard Hack.
Users: we found the routine, where (with high probability) the problem is, worked out patches, found the first version for the problem, etc.

Yes, I was a bit (too) pushy, but without this IMHO we were nowhere near now. You had 1,5 years to solve this issue on your own agenda, which brought what: literally nothing!

I really do not understand your attitude toward users trying to patch a security (!) regression (!): you are handling us as we were some criminals, etc. Instead of being glad to have such responsible users who are BTW doing your duties (the QA or an automated test should catch such security regressions even before publishing).

Don't worry, I will not comment on this bug anymore, or on any other one.
Comment 57 Matúš Kukan 2014-03-06 20:04:51 UTC
(In reply to comment #51)
> @VolkerTwer @Matúš Kukan @Markus Mohrhard
> Thanks for taking time for this issue!
> It seems we are only a small step away from a working patch!
> Unfortunately I cannot create one, but I think sg like this would be
> necessary:
> 
> Inside
> http://opengrok.libreoffice.org/xref/core/framework/source/services/
> autorecovery.cxx#implts_saveOneDoc
> 
> OUString sPassword =
> lOldArgs.getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_PASSWORD(),
> OUString());
> if (!sPassword.isEmpty())
>     lNewArgs[utl::MediaDescriptor::PROP_PASSWORD()] <<= sPassword;
> 
> + css::uno::Sequence< css::beans::NamedValue > aEncryptionData =
> getUnpackedValueOrDefault(utl::MediaDescriptor::PROP_ENCRYPTIONDATA(),
> css::uno::Sequence< css::beans::NamedValue >() );
> + if (aEncryptionData.getLength() > 0)
> +     lNewArgs[utl::MediaDescriptor::PROP_ENCRYPTIONDATA()] <<=
> aEncryptionData;
> 
> So PROP_ENCRYPTIONDATA would be put into lNewArgs too, when needed.
> 
> IMHO this issue in LibO was introduced this way:
> 1. m106 was merged, and LibO started to use PROP_ENCRYPTIONDATA.
>    At that time the recovery was working without problems, but bug 37825 was
> introduced.
> 2. Bug 37825 was fixed with these commits:
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=dd5f9610f5df4d6ac8062d7b94db0353f8c0fb72
> http://cgit.freedesktop.org/libreoffice/core/commit/
> ?id=cdfad2dbbf180d3c556964c7aa8e0bb3b299d5e3

Indeed, great analysis etc, I've tried your patch and it works!
Great catch!
I think you would like to contribute your patch ?
Can you create it with help of https://wiki.documentfoundation.org/Development/gerrit or just give me your name & e-mail adrress and I will create it for you.
And we would need your license statement too as described in https://wiki.documentfoundation.org/Development/Developers#Developers_and_Contributors_list
Add yourself to the developers list there or we will do it for you after the license statement.
Thanks a lot for helping LibreOffice by filling bugs and also helping to fix them!
Comment 58 Commit Notification 2014-03-17 19:43:41 UTC
Matuš Kukan committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=ef87ff6680f79362a431db6e7ef2f40cfc576219

fdo#51819: autorecovery: fix saving password in protected documents.



The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 59 stragu 2014-03-19 03:07:45 UTC
Dear all
Can I ask why this fix is not integrated into 4.1 and 4.2?
Comment 60 Joel Madero 2014-03-19 03:36:55 UTC
Because it needs additional testing to make it into 4.2 - I believe 4.1 is nearing EOL so making it into that branch might be a bit harder but I could be wrong.
Comment 61 Michael Meeks 2014-03-19 15:32:27 UTC
It's in gerrit for 4.2:

https://gerrit.libreoffice.org/#/c/8633/

needs one developer / reviewer and some testing I guess; I'd be happy to see people playing with autosave some across many components before shipping it personally. Luckily testing is something anyone can do with master builds for each component - it'd be great to get user feedback on setting a v. short (1 minute?) autosave time and doing some live editing of odt/odp/ods, doc/xls/xlsx etc. and seeing how it behaves.
Comment 62 Commit Notification 2014-03-20 00:15:19 UTC
Matuš Kukan committed a patch related to this issue.
It has been pushed to "libreoffice-4-2":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=827d7dbf3b75e61f63c769ce41634e890a608455&h=libreoffice-4-2

fdo#51819: autorecovery: fix saving password in protected documents.


It will be available in LibreOffice 4.2.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 63 Commit Notification 2014-03-21 11:35:37 UTC
Matuš Kukan committed a patch related to this issue.
It has been pushed to "libreoffice-4-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=f9be1da8203d610a038732c6c8a80206b038fa79&h=libreoffice-4-1

fdo#51819: autorecovery: fix saving password in protected documents.


It will be available in LibreOffice 4.1.6.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.
Comment 64 Commit Notification 2014-03-25 10:10:23 UTC
Matuš Kukan committed a patch related to this issue.
It has been pushed to "libreoffice-4-2-3":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7e1d17f1ba3776da70db40d6d1a518dcf3238860&h=libreoffice-4-2-3

fdo#51819: autorecovery: fix saving password in protected documents.


It will be available already in LibreOffice 4.2.3.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds
Affected users are encouraged to test the fix and report feedback.