Bug 1098934 (CVE-2018-12466) - VUL-0: CVE-2018-12466: obs: Delete a package in a project link exploit
Summary: VUL-0: CVE-2018-12466: obs: Delete a package in a project link exploit
Status: RESOLVED FIXED
Alias: CVE-2018-12466
Product: SUSE Security Incidents
Classification: Novell Products
Component: Incidents (show other bugs)
Version: unspecified
Hardware: Other Other
: P3 - Medium : Normal
Target Milestone: ---
Assignee: Björn Geuken
QA Contact: Security Team bot
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-25 05:08 UTC by Marcus Meissner
Modified: 2018-07-26 07:00 UTC (History)
9 users (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---
bgeuken: needinfo? (mls)


Attachments
delete_package_in_prjlink_exploit.txt (5.63 KB, text/plain)
2018-06-25 05:10 UTC, Marcus Meissner
Details
0001-Add-testcase-for-the-delete-package-exploit.patch (2.91 KB, patch)
2018-06-25 05:10 UTC, Marcus Meissner
Details | Diff
0002-Fix-the-delete-package-exploit.patch (2.21 KB, patch)
2018-06-25 05:11 UTC, Marcus Meissner
Details | Diff
0003-Add-testcase-for-checking-permissions-in-case-of-a-m.patch (1.89 KB, patch)
2018-06-25 05:12 UTC, Marcus Meissner
Details | Diff
0004-Perform-permission-checks-for-a-maintenance_incident.patch (1.07 KB, patch)
2018-06-25 05:12 UTC, Marcus Meissner
Details | Diff
patch for bsc#1098934 (65.89 KB, patch)
2018-06-28 13:33 UTC, Björn Geuken
Details | Diff
updated patch for bsc#1098934 (62.11 KB, patch)
2018-06-28 14:16 UTC, Björn Geuken
Details | Diff
updated patch for bsc#1098934 (65.46 KB, patch)
2018-06-28 14:20 UTC, Björn Geuken
Details | Diff
updated patch for bsc#1098934 (65.48 KB, patch)
2018-06-28 15:21 UTC, Björn Geuken
Details | Diff
updated patch for bsc#1098934 (100.32 KB, patch)
2018-06-29 15:31 UTC, Björn Geuken
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcus Meissner 2018-06-25 05:08:06 UTC
Hi,                                                                                                                                                                                          
                                                                                                                                                                                             
under some circumstances it is possible to delete a package if the package's                                                                                                                 
project is a link. The details can be found in the attached                                                                                                                                  
delete_package_in_prjlink_exploit.txt file. The attached patches fix the                                                                                                                     
problem in a quite conservative way (and break the testsuite). Hence, they                                                                                                                   
serve merely as a basis for discussion (I did not want to spend too much                                                                                                                     
on them until we agreed how to fix it).                                                                                                                                                      
                                                                                                                                                                                             
As requested by Adrian, I also CCed security@suse.de                                                                                                                                         
                                                                                                                                                                                             
Filename                          md5sum                                                                                                                                                     
e99568f881f50d488ccee1d4cb88fcce  0001-Add-testcase-for-the-delete-package-exploit.patch                                                                                                     
6b25e2d491b49b5d2fd7377fdfc6c752  0002-Fix-the-delete-package-exploit.patch                                                                                                                  
d5d552008492463e2bce038a035ed749  0003-Add-testcase-for-checking-permissions-in-case-of-a-m.patch                                                                                            
3e9a3fb3db7f1e379a9114f14f856748  0004-Perform-permission-checks-for-a-maintenance_incident.patch                                                                                            
d19d36d77e3928cf21b344d6ce23b058  delete_package_in_prjlink_exploit.txt                                                                                                                      
                                                                                                                                                                                             
                                                                                                                                                                                             
Marcus
Comment 1 Marcus Meissner 2018-06-25 05:10:14 UTC
Created attachment 775130 [details]
delete_package_in_prjlink_exploit.txt

delete_package_in_prjlink_exploit.txt

from Marcus Huewe
Comment 2 Marcus Meissner 2018-06-25 05:10:31 UTC
Created attachment 775131 [details]
0001-Add-testcase-for-the-delete-package-exploit.patch

0001-Add-testcase-for-the-delete-package-exploit.patch
Comment 3 Marcus Meissner 2018-06-25 05:11:40 UTC
Created attachment 775132 [details]
0002-Fix-the-delete-package-exploit.patch

0002-Fix-the-delete-package-exploit.patch
Comment 4 Marcus Meissner 2018-06-25 05:12:02 UTC
Created attachment 775133 [details]
0003-Add-testcase-for-checking-permissions-in-case-of-a-m.patch

0003-Add-testcase-for-checking-permissions-in-case-of-a-m.patch
Comment 5 Marcus Meissner 2018-06-25 05:12:33 UTC
Created attachment 775134 [details]
0004-Perform-permission-checks-for-a-maintenance_incident.patch

0004-Perform-permission-checks-for-a-maintenance_incident.patch
Comment 6 Marcus Meissner 2018-06-25 05:17:57 UTC
Please use CVE-2018-12466
Comment 7 Michael Schröder 2018-06-25 11:41:01 UTC
(without looking into the patch)

So they seem to be two things that need to be fixed:

1) the code should *never* delete a package coming over a project link

2) there seems to be a permission check missing at accept time (not sure about this)
Comment 8 Marcus Hüwe 2018-06-25 12:07:10 UTC
(In reply to Michael Schröder from comment #7)
> So they seem to be two things that need to be fixed:
> 
> 1) the code should *never* delete a package coming over a project link
> 
Probably a stupid question, but why should this be forbidden?
The package was explicitly created in the Base:Staging project. I don't see
a (obvious?) reason why it shouldn't be treated as an "ordinary" package.

(Note: the cleanup of the Base:Staging/foo package (regardless if it exists
or not) will _never_ result in a cleanup of the Base/foo package (at least wrt.
the current codebase))

> 2) there seems to be a permission check missing at accept time (not sure
> about this)

Maybe... this would be basically approach B) (or even a combination of A and B).
Comment 9 Björn Geuken 2018-06-28 13:33:01 UTC
Created attachment 775562 [details]
patch for bsc#1098934
Comment 10 Michael Schröder 2018-06-28 13:49:49 UTC
In reply to comment #8:

I must admit that I just talked to Adrian and didn't read your report. So that's why I thought the attack is to create a project link to some project you want to attack, create a local package that "shadows" some other package, create a submission to some other project with cleanup, delete the local package. If the request is accepted the package must not be deleted in the wrong project.

Hmm. Ok. You've got a different attack. But could someone please check that the scenario above also is caught? I.e. nothing gets deleted if I submit a package coming from a project link?
Comment 11 Björn Geuken 2018-06-28 14:16:22 UTC
Created attachment 775574 [details]
updated patch for bsc#1098934
Comment 12 Björn Geuken 2018-06-28 14:16:47 UTC
btw, Thanks a lot for the detailed description
Comment 13 Björn Geuken 2018-06-28 14:17:26 UTC
* in the delete_package_in_prjlink_exploit.txt file
Comment 14 Björn Geuken 2018-06-28 14:20:23 UTC
Created attachment 775576 [details]
updated patch for bsc#1098934
Comment 15 Marcus Hüwe 2018-06-28 14:41:21 UTC
(In reply to Björn Geuken from comment #14)
> Created attachment 775576 [details]
> updated patch for bsc#1098934

> +  before_save :check_permissions_for_sources
> +

Hmm why don't we hook this in the "standard" permission checking
codepath?

> +  def check_permissions_for_sources
> +    return unless action_type == :submit && (sourceupdate || updatelink)

We should also check this for a "maintenance_incident" type.
Do we really need to do this in the updatelink case? (See the comment
in the delete_package_in_prjlink_exploit.txt)

Also, this does not "resolve" the "<sourceupdate>update</sourceupdate>"
vs. no "sourceupdate" element "inconsistency" (I'm fine with keeping
the current behavior, but maybe we should state at least in the code
or in the commit message that this behavior is intended).
Comment 16 Björn Geuken 2018-06-28 15:00:03 UTC
> Hmm why don't we hook this in the "standard" permission checking
codepath?

This way it applies no matter how the request get's created. Ideally we would have a policy for that. But that would require some bigger changes which I want to avoid since this will be backported to 2.9.

> We should also check this for a "maintenance_incident" type.

I will add it.

> Do we really need to do this in the updatelink case?

Honestly, I don't know. That's why I don't want to revert this right now.

My guess would be that this is because the requester would set the updatelink, as far as I understand.

> Also, this does not "resolve" the "<sourceupdate>update</sourceupdate>"
vs. no "sourceupdate" element "inconsistency"

Let's take care of this separately as this is not related with the security issue.
Comment 17 Björn Geuken 2018-06-28 15:21:10 UTC
Created attachment 775587 [details]
updated patch for bsc#1098934
Comment 18 Björn Geuken 2018-06-28 15:40:26 UTC
(In reply to Michael Schröder from comment #10)
> In reply to comment #8:
> 
> I must admit that I just talked to Adrian and didn't read your report. So
> that's why I thought the attack is to create a project link to some project
> you want to attack, create a local package that "shadows" some other
> package, create a submission to some other project with cleanup, delete the
> local package. If the request is accepted the package must not be deleted in
> the wrong project.
> 
Doesn't work. Though there is a bug caused by not closing the request when the source packages gets deleted:

"
    # just remove one package
    source_package = source_project.packages.find_by_name!(self.source_package)
    source_package.commit_opts = { comment: bs_request.description, request: bs_request }
    source_package.destroy
    Package.source_path(self.source_project, self.source_package)
"

source project in this case was not the linked project.
Comment 19 Marcus Hüwe 2018-06-28 19:58:56 UTC
(In reply to Björn Geuken from comment #16)
> > Do we really need to do this in the updatelink case?
> 
> Honestly, I don't know. That's why I don't want to revert this right now.
> 
> My guess would be that this is because the requester would set the
> updatelink, as far as I understand.
> 
Yes, but it just affects the target package. I'm fine if we keep this
code for now, but we should reconsider it:)

> > Also, this does not "resolve" the "<sourceupdate>update</sourceupdate>"
> vs. no "sourceupdate" element "inconsistency"
> 
> Let's take care of this separately as this is not related with the security
> issue.

Hmm depends a bit on the POV: if (sourceupdate.nil? || sourceupdate == 'update')
and all conditions for the "sourceupdate" codepath are fulfilled (see
sub sourcecopy in bs_srcserver), then accepting the request modifies the
_link file in the source package.
Comment 20 Björn Geuken 2018-06-29 15:31:57 UTC
Created attachment 775746 [details]
updated patch for bsc#1098934

Updated the patch. Now only sourceupdate == noupdate will skip the check. I also dropped the updatelink case while I was on it.

Please have another look.

@mls Please also have a look. Since I am not familiar with the backend internals
Comment 21 Marcus Hüwe 2018-07-01 13:57:34 UTC
(In reply to Björn Geuken from comment #20)
> Created attachment 775746 [details]
> updated patch for bsc#1098934
> 
> Updated the patch. Now only sourceupdate == noupdate will skip the check. I
> also dropped the updatelink case while I was on it.
> 

> +  def check_permissions_for_sources
> +    return unless action_type.in?([:submit, :maintenance_incident]) &&
> +      (sourceupdate.nil? || sourceupdate != 'noupdate')
> +

Could be simplified to

return unless action_type.in?([:submit, :maintenance_incident]) && sourceupdate != 'noupdate'

+    source_object = Package.find_by_project_and_name(source_project, source_package) ||
+      Project.get_by_name(source_project)
+
+    raise LackingMaintainership if source_object && !User.current.can_modify?(source_object)

(source_object is always defined)

Unfortunately, this breaks
(a) the testsuite (since we are now performing the check even if
    sourceupdate == nil)
(b) submissions from remote packages (Project.get_by_name returns a String
    in case of a remote project, which yields to an exception in
    User.current.can_modify?)

The problem with (a) is that we are now a bit too restrictive. If the
following holds an update of the source_package _might_ happen:

- (sourceupdate.nil? || sourceupdate == 'update') AND
- source_package@source_rev is a link to target_package AND
- source_package@<rev at acceptance time> is a link to target_package

Hence, this suggests that we should perform the permission check at the
request acceptance time if the conditions from above hold. However, I'm also
fine with doing it a request creation time (in the worst case, we are still too
restrictive), if we add the additional link check (the first two conditions
from above).

About (b): a remote package is no problem because we can only submit a
plain or an expanded revision => the "update source package" code is never
executed.

So we probably end up with something like this:

if type is not SR/MI && sourceupdate != 'noupdate':
  NO_CHECKS
if (sourceupdate.nil? || sourceupdate == 'update') && LINK_CHECK && !remote:
  PERMISSION_CHECK  # skip this for a MI type? (see below)
if sourceupdate == 'cleanup':
  PERMISSION_CHECK

Btw., I just realized that in case of a maintenance_incident only
sourceupdate == 'cleanup' can occur... I'm not sure if the new checks break
it if sourceupdate.nil? holds (in doubt, we only perform these checks in case
of a SR).

Sorry for all this back and forth... :/
Comment 22 Johannes Segitz 2018-07-24 09:09:21 UTC
this is now in OBS:Server:2.9:Staging/obs-server, can we make the bug public so our checkers don't freak out?
Comment 23 Björn Geuken 2018-07-25 12:16:47 UTC
Sorry, I answered in https://bugzilla.suse.com/show_bug.cgi?id=1100217#c6
and forgot to reply here as well.

Meanwhile we released the new packages and appliances