Bugzilla – Bug 1098934
VUL-0: CVE-2018-12466: obs: Delete a package in a project link exploit
Last modified: 2018-07-26 07:00:29 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
Created attachment 775130 [details] delete_package_in_prjlink_exploit.txt delete_package_in_prjlink_exploit.txt from Marcus Huewe
Created attachment 775131 [details] 0001-Add-testcase-for-the-delete-package-exploit.patch 0001-Add-testcase-for-the-delete-package-exploit.patch
Created attachment 775132 [details] 0002-Fix-the-delete-package-exploit.patch 0002-Fix-the-delete-package-exploit.patch
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
Created attachment 775134 [details] 0004-Perform-permission-checks-for-a-maintenance_incident.patch 0004-Perform-permission-checks-for-a-maintenance_incident.patch
Please use CVE-2018-12466
(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)
(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).
Created attachment 775562 [details] patch for bsc#1098934
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?
Created attachment 775574 [details] updated patch for bsc#1098934
btw, Thanks a lot for the detailed description
* in the delete_package_in_prjlink_exploit.txt file
Created attachment 775576 [details] updated patch for bsc#1098934
(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).
> 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.
Created attachment 775587 [details] updated patch for bsc#1098934
(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.
(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.
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
(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... :/
this is now in OBS:Server:2.9:Staging/obs-server, can we make the bug public so our checkers don't freak out?
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