On some servers, the Update Manager allows administrators to directly execute arbitrary code even without the PHP module

Is the "administer software updates" permission listed as one of the permissions that affect security?

We're really afraid of people who leave themselves logged in as super admins and walk away from their machines at a coffee shop or something? Really? ;)

Anyway, if the workflow introduced by #609728: Skip authorize.php step if webroot files are owned by the httpd user is too permissive, we should be able to use authorize.php to ask the admin user to log in as themselves again. That'd satisfy the security concern here, right? Super rough sketch of how this would work:

(Note, this might depend on the API changes from #609772: Impossible to extend the FileTransfer class system in contrib before it actually works).

Instead of update.manager.inc directly creating a FileTransferLocal object in this case, it does special mojo to redirect to authorize.php in such a way that it's not prompting for FTP/SSH credentials, but basically a login form.

- It'd have to not call system_authorized_init(), but instead manually setup $_SESSION['authorize_filetransfer_backends'] and $_SESSION['authorize_operation'] itself.
- It'd setup authorize_filetransfer_backends to use a custom "settings form" and factory callback.
- The factory callback would basically validate the password field, and if it worked, it'd instantiate the right FileTransferLocal object to actually do the operation.
- The update/install would proceed as normal via authorize.php at this point

In fact, this last point is probably an important fix to #609772 in the update case. We actually *want* to run updates from authorize.php so we've got the low bootstrap level and have a slightly cleaner environment to operate in. It doesn't matter when installing new code, but when updating existing code, we don't want all modules bootstrapped. It's certainly evil and dumb to rely on FTP and ask for those credentials (which we don't need) if all the files are owned by the same user. But, we could verify that the user running the operation is a human that knows the password to an account with "administer software updates". Might be a bit of a UX pain in the ass. :( Especially since we never fixed #720452: Allow ability to install multiple modules at once :( Maybe there could be another settings.php killswitch for this behavior? OTOH, if they're using FTP, we ask them for their password each time, so at least it'd be consistent. In a way, that'd actually make it easier to document all this stuff, since the workflow would always be the same.

Anyway, if we actually want this approach, the first step is getting #609772 in, since the whole API for managing this stuff is completely broken in HEAD right now. I have a patch with verified working tests and squeeky-clean API docs over there, it just needs a review other than me before it's RTBC. Once that's done, we can proceed with the plan I outline here.

Meanwhile, it'd be nice to get some UX reviews of this proposed workflow...

This sounds like a won't fix at this point, I am not sure I fully understand but if it requires logging in again we surely are doing something wrong - asking FTP information should be enough. If the webserver allows things to pass without FTP information, are we to blame or the server configuration?

@Bojhan: This would be *instead* of entering your FTP credentials, but at the exact same spot in the workflow. The concern raised here is that due to #609728: Skip authorize.php step if webroot files are owned by the httpd user, on common shared hosting server configurations (where all the files are owned by the user that httpd is running as), if a novice admin leaves themselves logged into the site all the time, and they ever leave their browser unattended, any random person could use the update manager to install malicious PHP code on the site. One assumption we had previously made is that if you completely removed the PHP module, there's no way in Drupal core to execute arbitrary PHP from the web UI. Update manager currently breaks this assumption in many cases.

So, this issue is proposing that in the special case of #609728, when the update manager currently skips authorize.php entirely and *directly* installs/updates new code, we'd still land on authorize.php. The user would technically still be logged in, they'd just be asked to provide their site admin login credentials again (extremely common when installing software updates or new code in other contexts (e.g. Apple's "Software Update").

This way, you'd have to be so foolish as to leave your machine unlocked at a coffee shop *and* tape your Drupal admin password to your laptop for any random user to install malicious PHP code on your site, even without PHP module.

But I'm still torn. My security team hat says "fix this".

My "updates should never happen during a full bootstrap" hat says to fix it. However, most of the theory behind the low bootstrap authorize.php doesn't translate into much practice. The final authorize.php landing page doesn't actually let you recover from failure, restore your site, anything. It's just a last dying breath, potentially full of red errors, telling you "bummer, I just hosed your site, sorry", and the site is already WSOD.

My "updating my site should be easy" hat (the whole premise of the Update manager) says "just document this risk and leave all the code alone."

Another approach is to make it so that you could update existing modules without giving the site your FTP or SFTP information, but not install new ones. To clarify:

Situation 1)
All of my drupal files are owned by the same user and group as my web server. I can go update any module without FTP of SFTP information. In order to install a new module, I'd need to supply credentials.

Situation 2)
All of my drupal files have the ownership set in an intelligent manner. I must supply FTP or SFTP information in order to update existing modules, or install new ones.

Yes, that's certainly a good start. The only thing I'd consider adding is a link to http://drupal.org/server-permissions (although that page seems to have been reorganized into a more catch-all place, so that might not be helping much).

-1 for asking the account password to the user here.

Users using an external authentication mechanism (openid, single sign on, ssl authentication etc.) don't have a valid password known to drupal, so it's impossible to check for it.

The comment looks like the best we can do without building a complete authentication abstraction layer.

Patch in #9 still applies.

Is http://drupal.org/node/244924 the link it should include and what /server-permissions used to be?

As long as we're talking about a documentation-only change, this is a task.

Tagging issues not yet using summary template.

Well, let's get the documentation fix in, at least. (And I think the page in #10 isn't really useful and probably needs a different alias now at that.)

Tagging novice to reroll #9 against the current D8 codebase.

Hey guys, I rerolled #9 against the current D8 codebase.
Patch attached.

Adding DDU2012 I tag I omitted in #16 :)

DDU2012? What's that?

Drupal Down Under. :) They're having a code sprint today.

I think #16 is RTBC, since the handbook documentation is not-so-useful. We can set it back to active for other changes after.

Why is this still not in? Its stalled on a code comment? Sometimes I feel our major/critical change basically made all majors into normals.

Since the patch in #16 is for a documentation improvement, let's incorporate David's changes into that improvement. This would be a good novice task.

Patch updated with @David_Rothstein's suggestions. Unable to interdiff because previous patch no longer applies.

The patch from #23 doesn't apply anymore, so I re-rolled and attached it.

Re-rolled.

This is working fine for me.

Note: The patch for D7 will have to be re-rolled as patch #27 won't apply.

Looks good to me, and seems to incorporate the latest feedback.

Committed and pushed to 8.x. Moving to 7.x for backport. Also changing the documentation component, since this is purely a docs fix now.

Backported #27 to D7.

Looks like the same fix as was committed to d8, thanks for the backport!

Committed to 7.x (pure docs).

Err. Right. No idea what happened there. :P

RE-committed and pushed to 7.x. :P Forreal this time.

Yep. Taking a look at this it looks like there are two concurrent next steps here:

• 'a generic "reauthenticate-on-demand" capability in Drupal core' (per #33)
• update issue summary (in case it helps someone new, here is the contributor task help doc for that: http://drupal.org/node/1427826)

'a generic "reauthenticate-on-demand"' sounds kind of complicated. Who would be a good person to have the right skills to tackle that? maybe we could ping a couple people to see if they are interested. and/or some more detail about what that would entail might be helpful.

Seems like the only thing left here for 8.x is to revert the change as per #35?

A patch was committed to 8.x (#29) and to 7.x (#32) for this. The 7.x one was accidentally reverted but then restored (#36). This all points to this being completed.

It appears the only left is to create a followup for #33 and then this can be closed. Adding tag for a followup.