bug#19479: Package manager vulnerable

classic Classic list List threaded Threaded
45 messages Options
123
Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Kelly Dean
Stefan Monnier wrote:
> All this arguing just to try and avoid signing the standard document
> baffles me

If I sign an assignment document, then I would be committing perjury. Possibly in the legal sense, and at least in the moral sense.

And there isn't just one standard document. There are at least five; three for disclaimers, and two for assignments. I asked the clerk to choose the correct disclaimer for me. If he'd done it, none of this arguing would be necessary.

All of which I already pointed out, so I'm baffled by your bafflement.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Richard Stallman
In reply to this post by Kelly Dean
[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

In general, we do accept code whose copyright has been disclaimed by
the author.  That is not our preference, but it is ok.

Would you please discuss this privately with me and the copyright clerk?

--
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call.




Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Stefan Monnier
In reply to this post by Kelly Dean
> If I sign an assignment document, then I would be committing perjury.

No, the assignment document is just for the Emacs code you wrote and
whose copyright you own.  It simply doesn't apply to the code whose
copyright you don't own (which normally only happens when the copyright
is owned by your employer).


        Stefan



Reply | Threaded
Open this post in threaded view
|

bug#19479: (on-topic) Re: bug#19479: Package manager vulnerable

Kelly Dean
In reply to this post by Kelly Dean
Back on topic...

I found a good way to add timestamps to prevent metadata replay (the other vulnerability), and to further harden the package manager's security, but of course I'll wait until we hear from the clerk before trying to implement it.

The reason I said there's a compatibility problem for timestamps is that archive-contents is a list consisting just of a version number followed by a bunch of package records; the list's format isn't extensible (though the package record format is extensible). There's no way to insert a timestamp without changing the list's format (and thus, the version number), but if you do that, then old clients can't understand archive-contents anymore.

Even worse, old clients become stuck because they store the new-format (incompatible) file before checking the version number, then barf on it and refuse to accept even an old-format (compatible) file to replace it until you manually delete the stored one.

I see four possible solutions:
0. Have a flag day, on which all the elpas switch to the new format, and on or before which everybody must upgrade to Emacs 25 or his package manager will stop working.
1. Have the server check the User-Agent header, and send the old-format file if it's ⌜URL/Emacs⌝, and the new-format if it's ⌜URL/Emacs-25⌝ or later.
2. Use a different URL for the new-format file.
3. Keep the old format, and put the timestamp in a different file.

#0 obviously isn't an option.
I advise against #1, for reasons which everybody here already knows.
#2 would work, but is inelegant, since you would still have to retain the old-format file for the sake of old clients, and it's inefficient, since new clients would have to periodically re-download the entire file (fairly big, in Melpa's case) even if nothing but the timestamp changed (and clients have to demand fresh timestamps in order to prevent metadata replay attacks).

#3 looks like the best solution. The timestamp file includes the timestamp and the hash of archive-contents. Sign the timestamp file for the sake of new clients.

Old clients would ignore the timestamp file. If archive-contents is unchanged, then new clients would only have to periodically re-download the timestamp file and signature--the minimal amount of data necessary. They'd see that the current hash of archive-contents matches the version they already have stored. IOW, to whoever made archive-contents inextensible: thank you! You've forced the right solution to timestamping. ;-)

Combined with my previous patch, this leaves the timestamp-file's signature as the only one that's necessary to secure the entire archive (packages and metadata, including timestamp) and prevent both package and metadata replay attacks. IMHO, this simplicity makes it practical to insist that all elpas provide this signature, so Emacs 25 could enforce it by default.

Optionally continue signing archive-contents for the sake of 24.4 clients, but since 25 won't need that signature, nothing before 24.4 is capable of checking it, 24.4 doesn't enforce it by default, Melpa doesn't even provide it IIUC (GNU Elpa does), and 24.4 is vulnerable to package and metadata replay anyway, you might as well not. The kind of people who have changed package-check-signature to t will upgrade to 25 anyway.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Kelly Dean
In reply to this post by Stefan Monnier
Stefan Monnier wrote:
>> If I sign an assignment document, then I would be committing perjury.
>
> No, the assignment document is just for the Emacs code you wrote and
> whose copyright you own.  It simply doesn't apply to the code whose
> copyright you don't own (which normally only happens when the copyright
> is owned by your employer).

I don't have a copy of it handy, but if that's the way it's worded, then you're right, it wouldn't be illegal for me to sign it. It would only be immoral. It would be legally vacuous, and deceptive, leading to doubt about my intent.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Werner LEMBERG

>> No, the assignment document is just for the Emacs code you wrote
>> and whose copyright you own.  It simply doesn't apply to the code
>> whose copyright you don't own (which normally only happens when the
>> copyright is owned by your employer).
>
> I don't have a copy of it handy, but if that's the way it's worded,
> then you're right, it wouldn't be illegal for me to sign it. It
> would only be immoral. It would be legally vacuous, and deceptive,
> leading to doubt about my intent.

Perhaps slightly off-topic, but quite relevant IMHO in a global
perspective:

It is *not* possible to create not-copyrighted code in some
jurisdictions like Germany.  Whatever you write, it is by default
copyrighted by you (regardless whether there is a `public domain' line
or not), and you have to explicitly disclaim or reassign the
copyright.

For this reason, it is *much* better to use a license like CC0 instead
of a public domain notice, since this covers the `public domain' idea
in virtually all countries.  Actually, this is what the FSF recommends
(https://www.gnu.org/licenses/license-list.en.html#CC0), and I guess
this works for emacs also, since it works already for GNU (according
to https://gcc.gnu.org/contribute.html#legal).


    Werner



Reply | Threaded
Open this post in threaded view
|

bug#19479: Copyright issue

Richard Stallman
In reply to this post by Stefan Monnier
[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

Instead of having a discussion here, please let me get this worked out
between Kelly and our clerk, with the help of lawyers when needed.

--
Dr Richard Stallman
President, Free Software Foundation
51 Franklin St
Boston MA 02110
USA
www.fsf.org  www.gnu.org
Skype: No way! That's nonfree (freedom-denying) software.
  Use Ekiga or an ordinary phone call.




Reply | Threaded
Open this post in threaded view
|

bug#19479: Disclaimer is now on file at FSF

Kelly Dean
In reply to this post by Kelly Dean
The FSF has accepted my disclaimer and added me to their records. You can install my patches if you find them satisfactory on technical grounds.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Emacs package manager vulnerable to replay attacks

Kelly Dean
In reply to this post by Kelly Dean
Note, I'm not implementing the metadata-replay fix, because it's unlikely my patch would be accepted, so somebody else will need to do it. See my January 11th message to bug #19479 for a description of how to do it.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Disclaimer is now on file at FSF

Glenn Morris-3
In reply to this post by Kelly Dean

So, I don't want to get into this discussion, but I've always assumed
that disclaimers do not/cannot apply to future changes. I asked
[hidden email], and they confirmed "Disclaimers are not valid for future
contributions".

I mention this because AFAICS you are sending new patches.

Your disclaimer is dated 2015-1-8. AFAICS we cannot apply anything after
that. Someone should also check the several patches from you that have
been applied recently to make sure they originated before this date.

Sorry, I don't have time/inclination to discuss special cases.
Maybe you want to take it up with rms and/or assign@gnu.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Disclaimer is now on file at FSF

Kelly Dean
> So, I don't want to get into this discussion,

And yet you chose to dig it back up, even after everybody else was satisfied that it was resolved more than a month ago. The copyright clerk's exact words on January 20th were, ‟We've accepted the public domain disclaimer and added you to our records”, not ‟we've accepted part of the disclaimer, but rejected another part”.

The disclaimer covers future changes, and everybody's comments about that part had already been CCed to the clerk, and his answer was, ‟We've accepted the public domain disclaimer”.

> but I've always assumed
> that disclaimers do not/cannot apply to future changes. I asked
> [hidden email], and they confirmed "Disclaimers are not valid for future
> contributions".

Good luck finding a copyright judge anywhere in America who would agree with your absurd claim that my work since January 8th is not in the public domain, despite my signed statement that it is.

Or if you admit it is PD, I'm sure you can dream up some rationalization of why PD code isn't allowed in Emacs, and then try to remove it all, which is a lot more than just my code.

Either way, I'm done trying to work on Emacs. This B.S. isn't worth my time.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Disclaimer is now on file at FSF

Glenn Morris-3
Kelly Dean wrote:

>> So, I don't want to get into this discussion,
>
> And yet you chose to dig it back up, even after everybody else was
> satisfied that it was resolved more than a month ago.

I've been largely on a break from Emacs. I always thought there was
something strange here, and I just happened to get motivated enough now
to ask assign@gnu for clarification when I saw patches were still arriving.

> The copyright clerk's exact words on January 20th were, ‟We've
> accepted the public domain disclaimer and added you to our records",
> not ‟we've accepted part of the disclaimer, but rejected another
> part".

I specifically mentioned you by name in the question I asked assign@gnu,
and the reply I got (one day ago) was, in totality:

   Disclaimers are not valid for future contributions. Thanks for checking in.

Like I said, you can take it up with them if you disagree.
I'be glad to be corrected, but it all seems pretty clear to me.

I am not trying to be the bad guy and I am not out to get you.
I applied several patches from you and would have been happy to apply more.
I am just trying to ensure Emacs follows the FSF's procedures,
which seem pretty clear to me.


Vibhav Pant wrote:

> Well, what about
> http://git.savannah.gnu.org/cgit/gnulib.git/plain/doc/Copyright/request-assign.future?
> This is the form to request documents for assigning past and future
> works, which according to you isn't possible.

That's not what I said.
I said: I am told *disclaimers* cannot apply to future changes.

You will note that there are separate documents for *assigning* past
changes (request-assign.changes), and past-and-future changes
(request-assign.future). But for *disclaimers* there is only
request-disclaim.changes. There is no request-disclaim.future.
If you read

http://git.savannah.gnu.org/cgit/gnulib.git/tree/doc/Copyright/request-disclaim.changes

it quite clearly states that it only applies to past, finished changes.

I am not a lawyer (AFAIK, neither is anyone else on this list) and
have no interest in discussing why these things are different.
They just are.

I said the first time we went through this that it was my
understanding that disclaimers worked this way. I said it again here:
https://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00198.html

I have now had this confirmed by assign@gnu.


If you (the generic you) want to contribute to Emacs, there is a
well-defined, simple procedure that hundreds of people have followed
with no problem.

If you don't want to follow the procedure, then fine, that's your
prerogative. Then you can't contribute.

But please don't start arguing with us about what the procedure is, or
should be, or what you think a judge might say, or why you need to be an
exception. We don't set the rules here at Emacs, and it just isn't a
productive use of anyone's time.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Disclaimer is now on file at FSF

Eli Zaretskii
unblock 24655 by 19479
thanks

> From: Glenn Morris <[hidden email]>
> Date: Wed, 25 Feb 2015 16:09:57 -0500
> Cc: [hidden email], [hidden email]
>
> I am not a lawyer (AFAIK, neither is anyone else on this list) and
> have no interest in discussing why these things are different.
> They just are.
>
> I said the first time we went through this that it was my
> understanding that disclaimers worked this way. I said it again here:
> https://lists.gnu.org/archive/html/emacs-devel/2015-01/msg00198.html
>
> I have now had this confirmed by assign@gnu.
>
>
> If you (the generic you) want to contribute to Emacs, there is a
> well-defined, simple procedure that hundreds of people have followed
> with no problem.
>
> If you don't want to follow the procedure, then fine, that's your
> prerogative. Then you can't contribute.
>
> But please don't start arguing with us about what the procedure is, or
> should be, or what you think a judge might say, or why you need to be an
> exception. We don't set the rules here at Emacs, and it just isn't a
> productive use of anyone's time.

Two and a half years later, with no one complaining about this, it
doesn't sound right for this issue to block the release of Emacs 26.1.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Glenn Morris-3
In reply to this post by Kelly Dean

[Dropping emacs-devel, since that seems unlikely to be productive given
the lack of context.]

Eli Zaretskii wrote:

> Two and a half years later, with no one complaining about this, it
> doesn't sound right for this issue to block the release of Emacs 26.1.

The context here was security vulnerabilities in the package manager.
Personally I'm uneasy with saying "we've ignored this for X years so
let's continue to ignore it.". But I don't have anything substantive to
add.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Stefan Kangas
In reply to this post by Kelly Dean
Kelly Dean <[hidden email]> writes:

> Ivan Shmakov requested that I send this message to the bug list.
>
> For details, see my message with subject ⌜Emacs package manager vulnerable to replay attacks⌝ to emacs-devel on 30 Dec 2014:
> https://lists.gnu.org/archive/html/emacs-devel/2014-12/msg02319.html
>
> Executive summary to fix the vulnerabilities:
>
> 0. Include a hash and length of each package's content in the package's record
> in archive-contents, rather than only including the package name and version
> number in that file as Emacs currently does. Barf if a package hash doesn't
> verify, regardless of whether any signatures verify.
> (Length technically not necessary, but still generally useful, e.g. if
> there's a length mismatch then you know there's a content mismatch and
> you don't have to bother checking the hash.)
I have implemented the first part of the protection against metadata
replay attacks in the attached patch: support for checksum (or hash)
verification.  This change is backwards-compatible; the new fields can
be added to "archive-contents" file without impacting old clients.
I've not yet updated documentation, NEWS, etc. but will get to that
next.

I introduce a new user option `package-verify-checksums' that controls
this new behaviour.  The default is 'allow-missing', which only
carries out this check if there are checksums in "archive-contents",
and does nothing otherwise.  In itself, this does nothing to protect
against metadata replay attacks (but might protect against data
corruption).  You need to set `package-verify-checksums' to t, and
implement timestamping (discussed below).

I still suggest to stick with this default for Emacs 27.1, or at least
until common package archives can catch up.  Once this is implemented
in GNU ELPA and MELPA, it makes more sense to move to a stricter
default.  Otherwise, the transition will be very bumpy.  I therefore
suggest to discuss stricter defaults later.

(BTW, I didn't bother fixing the package-x.el code for this patch,
since it seems like it's not that widely used.  It will work as
before, but lack support for adding the checksums automatically.)

> 1. Include a timestamp of archive-contents in that file itself (so that the
> signature in archive-contents.sig depends on the timestamp, so that the
> timestamp can't be forged), and have Emacs ignore any new archive-contents
> that's older than the latest valid one that Emacs has already seen or is older
> than some specified limit. One thing I forgot to mention in my original message:
> have Emacs signal a warning if it ever sees an archive-contents dated in the
> future, which indicates misconfiguration of the client or server (or of course,
> some kind of mischief).

To protect against metadata replay attacks, it is correct that we need
timestamps too.  I haven't done that in this first patch, but I hope
to do it in a following patch.  I wanted to get this first part done
before I started working on that.

My current best idea for how to do it is one which AFAICT haven't been
raised in this thread before: to add a comment with an RFC3339
timestamp to the top of the "archive-contents" file:

    ;; Last-Updated: 2019-10-01T15:32:55.000Z

This will be ignored by older versions of Emacs, since package.el uses
(read (current-buffer)) to read this file.  New versions will have
an easy time parsing this header, caching the value, and refusing to
update the package cache if the timestamp is older than one we have
already seen.  With that, we would have implemented protection
against metadata replay attacks.

I think it would be highly beneficial if this could go in before Emacs
27, not least so that package archives can start implementing support
for this.

Comments on all this are obviously more than welcome.

Best regards,
Stefan Kangas

PS. Note that the original thread ended up highly off-topic discussing
copyright issues, because one potential contributor refused to sign
the standard copyright assignment.  The eventual outcome was that we
could not use a patch written by that person.  I have therefore
deliberately not looked at that persons patch in order to avoid any
copyright issues.  I have implemented this from scratch based solely
on the below link, and the discussion in this thread:
https://www2.cs.arizona.edu/stork/packagemanagersecurity/attacks-on-package-managers.html

0001-Support-package-checksum-verification.patch (39K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Noam Postavsky
Stefan Kangas <[hidden email]> writes:

> Subject: [PATCH] Support package checksum verification
>
> This is the first step towards protecting users of package.el against
> metadata replay attacks.

> +(define-error 'bad-checksum "Failed to verify checksum")

Would it be useful to have bad-signature and this one share a parent?
(by the way, I kind of wonder why it's not called
package-bad-signature).

> +  (cl-flet*
> +      ((supported-hashes
> +        (lambda ()

Is this a function (rather than a variable) just so it can be in the
same cl-flet* as do-check?

> +          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))

The list returned by secure-hash-algorithms includes sha1 and md5.  This
is a problem if we're going to rely on signing them.  We should at least
plan to have some way of filtering out some functions.

> +                   (a (cdr hash))
> +                   (b (secure-hash algorithm (current-buffer))))

> +  (when-let ((a (package-desc-size pkg-desc))
> +             (b (string-bytes (buffer-string))))

I risk descending into trivial nitpicking, but I think 'a' and 'b' are
bit too generic.  Something like 'expected' and 'actual' would make it
harder to mix them up.

> +(defmacro run-verify-checksums-test (verify-checksums checksums)
> +  "Run a test for `package-verify-checksums'."

> +(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()

I think run-verify-checksums-test should be prefixed with package-test
(whereas the individual test names could be prefixed with just package).



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Stefan Kangas
Noam Postavsky <[hidden email]> writes:

> Stefan Kangas <[hidden email]> writes:
>
>> Subject: [PATCH] Support package checksum verification
>>
>> This is the first step towards protecting users of package.el against
>> metadata replay attacks.
>
>> +(define-error 'bad-checksum "Failed to verify checksum")
>
> Would it be useful to have bad-signature and this one share a parent?
> (by the way, I kind of wonder why it's not called
> package-bad-signature).

Indeed, I fixed that.

>> +  (cl-flet*
>> +      ((supported-hashes
>> +        (lambda ()
>
> Is this a function (rather than a variable) just so it can be in the
> same cl-flet* as do-check?

I'm not sure I understand; it should be a function instead of a variable
because there is logic in there to match `(secure-hash-algorithms)'
against `(package-desc-checksums pkg-desc)' and signal an error.

>> +          (or (seq-filter (lambda (h) (memql (car h) (secure-hash-algorithms)))
>
> The list returned by secure-hash-algorithms includes sha1 and md5.  This
> is a problem if we're going to rely on signing them.  We should at least
> plan to have some way of filtering out some functions.

Yes, we currently would place the onus on the package archives to not
use those algorithms.  We could choose to filter them out as completely
unacceptable, I think that makes sense.

>> +                   (a (cdr hash))
>> +                   (b (secure-hash algorithm (current-buffer))))
>
>> +  (when-let ((a (package-desc-size pkg-desc))
>> +             (b (string-bytes (buffer-string))))
>
> I risk descending into trivial nitpicking, but I think 'a' and 'b' are
> bit too generic.  Something like 'expected' and 'actual' would make it
> harder to mix them up.

Thanks, fixed.

>> +(defmacro run-verify-checksums-test (verify-checksums checksums)
>> +  "Run a test for `package-verify-checksums'."
>
>> +(ert-deftest package-test--verify-package-checksums-nil/ignore-invalid ()
>
> I think run-verify-checksums-test should be prefixed with package-test
> (whereas the individual test names could be prefixed with just package).

That's true.  Fixed.

Thanks for the review!

Best regards,
Stefan Kangas



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Noam Postavsky
Stefan Kangas <[hidden email]> writes:

>> Is this a function (rather than a variable) just so it can be in the
>> same cl-flet* as do-check?
>
> I'm not sure I understand; it should be a function instead of a variable
> because there is logic in there to match `(secure-hash-algorithms)'
> against `(package-desc-checksums pkg-desc)' and signal an error.

Ah, I think had forgotten about/was confused by cl-flet's (FUNC (lambda
ARGLIST ...)) syntax when I wrote that.  Although I suppose you could
make it a plain variable by moving it inside do-check's lambda (not sure
if that's an improvement)?



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Noam Postavsky
In reply to this post by Kelly Dean
Stefan Kangas <[hidden email]> writes:
>
>> One more feature: include in each version of archive-contents a hash
[...]
> Does anyone understand how this would improve security in our case?
> AFAIU, it can help with APT since they support distributing package
> metadata in several files.  ELPA uses only one file, so I'm not sure it
> would make much of a difference?

Not entirely, but there's a bit more detail on the emacs-devel thread
linked from the OP:

    One final feature that isn't necessary for preventing any of the
    vulnerabilities above, but still is helpful to make the historical record even
    more clear, is to include in each version of archive-contents a hash (and
    length) of the previous version of that file. This further constrains an
    attacker who has compromised the elpa key; he can still launch attacks, but
    it's harder to keep the attacks secret for very long, since he's forced to
    cause a fork in what's supposed to be a linear hash chain.

I think the idea is that if the attacker has the signing key and sends
out a bad version of archive-contents, it will be revealed as soon as
the victim gets a "good" version, since its previous-version hash won't
match.  Except that only works if the user can expect to get all
versions of archive-contents, so maybe I've missed something.



Reply | Threaded
Open this post in threaded view
|

bug#19479: Package manager vulnerable

Stefan Kangas
Noam Postavsky <[hidden email]> writes:

> I think the idea is that if the attacker has the signing key and sends
> out a bad version of archive-contents, it will be revealed as soon as
> the victim gets a "good" version, since its previous-version hash won't
> match.

Yes, this is what I understood to be the case as well.

> Except that only works if the user can expect to get all versions of
> archive-contents, so maybe I've missed something.

Exactly my point.  So we can't rely on it to bail out if the hashes
don't match up, I think.



123