[PATCH] setup: allow running as non-admin

[PATCH] setup: allow running as non-admin

[Shaddy Baddah]

[-- Attachment #1: Type: text/plain, Size: 128 bytes --]

Hi,

As discussed, please find the patch to allow setup to be run as a
non-admin user include clear text.

-- 
Regards,
Shaddy


[-- Attachment #2: setup-sans-admin-november-02-bare.patch --]
[-- Type: text/x-patch, Size: 7254 bytes --]

2013-11-06  Shaddy Baddah <lithium-cygwin at shaddybaddah dot name>

	* LogFile.cc (LogFile::flushAll): New function to flush log all logging to
	files without exiting (as LogFile::exit does).
	* LogFile.h: Declare new method closeAll.
	* main.cc (NoAdminOption): Add new CLI options -B/--no-admin. This option
	allows the user to suppress privilege elevation (in tandem with
	"asInvoker" requestedExecutionLevel changes to exe manifests).
	(WinMain): check if setup run with Administrator privilege and if the
	NoAdminOption has not been specified, attempt to elevate privilege to an
	Administrator via WINAPI ShellExecuteEx().
	* setup.exe.manifest: Add requestedExecutionLevel of asInvoker to allow
	suppression of privilege elevation.
	* setup64.exe.manifest: Modify requestedExecutionLevel from
	requireAdministrator to asInvoker to allow suppression of privilege
	elevation. Continuity of privilege elevation attempt on startup is
	implemented by main.cc changes to WinMain().
	* win32.cc (NTSecurity::isRunAsAdmin): New function to allow main.cc to
	check if setup.exe has been run with privilege elevated to Administrator
	level.
	* win32.h: Declare new method isRunAsAdmin.

diff --git a/LogFile.cc b/LogFile.cc
index 53c6ed7..ff8e260 100644
--- a/LogFile.cc
+++ b/LogFile.cc
@@ -149,6 +149,18 @@ LogFile::exit (int const exit_code)
 }
 
 void
+LogFile::flushAll ()
+{
+  log (LOG_TIMESTAMP) << "Writing messages to log files without exiting" << endLog;
+
+  for (FileSet::iterator i = files.begin();
+       i != files.end(); ++i)
+    {
+      log_save (i->level, i->key, i->append);
+    }
+}
+
+void
 LogFile::log_save (int babble, const std::string& filename, bool append)
 {
   static int been_here = 0;
diff --git a/LogFile.h b/LogFile.h
index 912d2c5..3bed1d6 100644
--- a/LogFile.h
+++ b/LogFile.h
@@ -32,6 +32,7 @@ public:
    * but doesn't call generic C++ destructors
    */
   virtual void exit (int const exit_code) __attribute__ ((noreturn));
+  virtual void flushAll ();
   virtual ~LogFile();
   // get a specific verbosity stream.
   virtual std::ostream &operator() (enum log_level level);
diff --git a/main.cc b/main.cc
index d4c6828..2be633e 100644
--- a/main.cc
+++ b/main.cc
@@ -35,6 +35,7 @@ static const char *cvsid =
 #define _WIN32_WINNT 0x0501
 #include "win32.h"
 #include <commctrl.h>
+#include <shellapi.h>
 #include "shlobj.h"
 
 #include <stdio.h>
@@ -93,6 +94,7 @@ HINSTANCE hinstance;
 static StringOption Arch ("", 'a', "arch", "architecture to install (x86_64 or x86)", false);
 static BoolOption UnattendedOption (false, 'q', "quiet-mode", "Unattended setup mode");
 static BoolOption PackageManagerOption (false, 'M', "package-manager", "Semi-attended chooser-only mode");
+static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for and enforce running as Administrator");
 static BoolOption HelpOption (false, 'h', "help", "print help");
 static BOOL WINAPI (*dyn_AttachConsole) (DWORD);
 static BOOL WINAPI (*dyn_GetLongPathName) (LPCTSTR, LPTSTR, DWORD);
@@ -289,6 +291,57 @@ WinMain (HINSTANCE h,
 						<< "\nCommand Line Options:\n");
     else
       {
+	OSVERSIONINFO version;
+	version.dwOSVersionInfoSize = sizeof version;
+	GetVersionEx (&version);
+	if ((version.dwMajorVersion >= 6)
+			&& !NoAdminOption && !nt_sec.isRunAsAdmin ())
+	  {
+		log (LOG_PLAIN) << "Attempting to elevate to Administrator" << endLog;
+		char exe_path[MAX_PATH];
+		if (!GetModuleFileName(NULL, exe_path, ARRAYSIZE(exe_path)))
+		  {
+			log (LOG_TIMESTAMP) << "GetModuleFileName() failed: " << GetLastError () << endLog;
+			goto finish_up;
+		  }
+
+		SHELLEXECUTEINFO sei = { sizeof(sei) };
+		sei.lpVerb = "runas";
+		sei.lpFile = exe_path;
+		sei.nShow = SW_NORMAL;
+
+        // Note, this is necessary to avoid an infinite loop.
+        // The understanding is that pre-Vista, the runas verb will not
+		// result in a privilege elevated process. Therefore we need to
+		// indicate to the forked process that it should be happy with
+		// whatever privileges it is run with.
+		std::string command_line_cs (command_line);
+		command_line_cs += " -";
+		command_line_cs += NoAdminOption.shortOption();
+		sei.lpParameters = command_line_cs.c_str ();
+
+		// avoid the ambiguity of having both the parent and child
+		// process logging simultaneously, by ending it here. perhaps
+		// overkill, but safe.
+		theLog->flushAll ();
+		theLog->clearFiles ();
+
+		if (!ShellExecuteEx(&sei))
+		  {
+			// Note: if user declined, we get an ERROR_CANCELLED.
+			// Merely to be compatible with existing Cygwin Setup
+			// behaviour. we do nothing with it but just exit.
+			// Future improvement is possible here.
+
+			// TODO: because we have been prudent and closed off the
+			// logging, we can't actually log in this way. Though it
+			// would be helpful
+//			log (LOG_TIMESTAMP) << "ShellExecuteEx() failed: " << GetLastError () << endLog;
+		  }
+		// once we are set on a course to privilege elevate, the parent
+		// process is unnecessary
+		goto finish_up;
+	  }
 	UserSettings Settings (local_dir);
 
 	main_display ();
@@ -296,6 +349,7 @@ WinMain (HINSTANCE h,
 	Settings.save ();	// Clean exit.. save user options.
       }
 
+finish_up:
     if (rebootneeded)
       {
 	theLog->exit (IDS_REBOOT_REQUIRED);
diff --git a/setup.exe.manifest b/setup.exe.manifest
index c195ebc..394f19f 100755
--- a/setup.exe.manifest
+++ b/setup.exe.manifest
@@ -19,6 +19,13 @@
 	  />
       </dependentAssembly>
   </dependency>
+  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
+    <security>
+      <requestedPrivileges>
+        <requestedExecutionLevel level="asInvoker" uiAccess="false"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
   <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
     <application>
       <!--The ID below indicates application support for Windows Vista -->
diff --git a/setup64.exe.manifest b/setup64.exe.manifest
index 61c5241..f0bf282 100755
--- a/setup64.exe.manifest
+++ b/setup64.exe.manifest
@@ -22,7 +22,7 @@
   <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
     <security>
       <requestedPrivileges>
-        <requestedExecutionLevel level="requireAdministrator" uiAccess="false"/>
+        <requestedExecutionLevel level="asInvoker" uiAccess="false"/>
       </requestedPrivileges>
     </security>
   </trustInfo>
diff --git a/win32.cc b/win32.cc
index 31a923d..11e323d 100644
--- a/win32.cc
+++ b/win32.cc
@@ -400,6 +400,18 @@ NTSecurity::setDefaultSecurity ()
     setAdminGroup ();
 }
 
+bool
+NTSecurity::isRunAsAdmin ()
+{
+  BOOL is_run_as_admin = FALSE;
+  if (!CheckTokenMembership(NULL, administratorsSID.theSID (), &is_run_as_admin))
+  {
+	  NoteFailedAPI("CheckTokenMembership(administratorsSID)");
+  }
+  return (is_run_as_admin == TRUE);
+}
+
+
 VersionInfo::VersionInfo ()
 {
   v.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
diff --git a/win32.h b/win32.h
index 7838dcc..91ff184 100644
--- a/win32.h
+++ b/win32.h
@@ -132,6 +132,7 @@ public:
   void resetPrimaryGroup();
   void setAdminGroup ();
   void setDefaultSecurity();
+  bool isRunAsAdmin ();
 private:
   void NoteFailedAPI (const std::string &);
   bool wellKnownSIDsinitialized () const { return _wellKnownSIDsinitialized; }

Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Thu, Nov 07, 2013 at 10:52:00AM +1100, Shaddy Baddah wrote:
>Hi,
>
>As discussed, please find the patch to allow setup to be run as a
>non-admin user include clear text.
>
>-- 
>Regards,
>Shaddy
>

Only a very minor comment:

>+
>+        // Note, this is necessary to avoid an infinite loop.
>+        // The understanding is that pre-Vista, the runas verb will not

This indentation looks wrong.

>+		// result in a privilege elevated process. Therefore we need to
>+		// indicate to the forked process that it should be happy with
>+		// whatever privileges it is run with.

cgf

Re: [PATCH] setup: allow running as non-admin

[Shaddy Baddah]

Hi Christopher,

On Nov 07 11:23, Christopher Faylor wrote:
> This indentation looks wrong.

Should I correct and republish?

-- 
Regards,
Shaddy

Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Thu, Nov 07, 2013 at 11:25:37AM +1100, Shaddy Baddah wrote:
>Hi Christopher,
>
>On Nov 07 11:23, Christopher Faylor wrote:
>> This indentation looks wrong.
>
>Should I correct and republish?

Yes, please.

cgf

Re: [PATCH] setup: allow running as non-admin

[Shaddy Baddah]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

Hi,


On Nov 07 11:23, Christopher Faylor wrote:
>> Hi,
>>
>> As discussed, please find the patch to allow setup to be run as a
>> non-admin user include clear text.
>>
>> --
>> Regards,
>> Shaddy
>>
>
> Only a very minor comment:
>
>> +
>> +        // Note, this is necessary to avoid an infinite loop.
>> +        // The understanding is that pre-Vista, the runas verb will not
>
> This indentation looks wrong.

Corrected with this updated patch.

-- 
Regards,
Shaddy



[-- Attachment #2: setup-sans-admin-november-03-bare.patch --]
[-- Type: text/x-patch, Size: 7242 bytes --]

2013-11-06  Shaddy Baddah <lithium-cygwin at shaddybaddah dot name>

	* LogFile.cc (LogFile::flushAll): New function to flush log all logging to
	files without exiting (as LogFile::exit does).
	* LogFile.h: Declare new method closeAll.
	* main.cc (NoAdminOption): Add new CLI options -B/--no-admin. This option
	allows the user to suppress privilege elevation (in tandem with
	"asInvoker" requestedExecutionLevel changes to exe manifests).
	(WinMain): check if setup run with Administrator privilege and if the
	NoAdminOption has not been specified, attempt to elevate privilege to an
	Administrator via WINAPI ShellExecuteEx().
	* setup.exe.manifest: Add requestedExecutionLevel of asInvoker to allow
	suppression of privilege elevation.
	* setup64.exe.manifest: Modify requestedExecutionLevel from
	requireAdministrator to asInvoker to allow suppression of privilege
	elevation. Continuity of privilege elevation attempt on startup is
	implemented by main.cc changes to WinMain().
	* win32.cc (NTSecurity::isRunAsAdmin): New function to allow main.cc to
	check if setup.exe has been run with privilege elevated to Administrator
	level.
	* win32.h: Declare new method isRunAsAdmin.

diff --git a/LogFile.cc b/LogFile.cc
index 53c6ed7..ff8e260 100644
--- a/LogFile.cc
+++ b/LogFile.cc
@@ -149,6 +149,18 @@ LogFile::exit (int const exit_code)
 }
 
 void
+LogFile::flushAll ()
+{
+  log (LOG_TIMESTAMP) << "Writing messages to log files without exiting" << endLog;
+
+  for (FileSet::iterator i = files.begin();
+       i != files.end(); ++i)
+    {
+      log_save (i->level, i->key, i->append);
+    }
+}
+
+void
 LogFile::log_save (int babble, const std::string& filename, bool append)
 {
   static int been_here = 0;
diff --git a/LogFile.h b/LogFile.h
index 912d2c5..3bed1d6 100644
--- a/LogFile.h
+++ b/LogFile.h
@@ -32,6 +32,7 @@ public:
    * but doesn't call generic C++ destructors
    */
   virtual void exit (int const exit_code) __attribute__ ((noreturn));
+  virtual void flushAll ();
   virtual ~LogFile();
   // get a specific verbosity stream.
   virtual std::ostream &operator() (enum log_level level);
diff --git a/main.cc b/main.cc
index d4c6828..127b279 100644
--- a/main.cc
+++ b/main.cc
@@ -35,6 +35,7 @@ static const char *cvsid =
 #define _WIN32_WINNT 0x0501
 #include "win32.h"
 #include <commctrl.h>
+#include <shellapi.h>
 #include "shlobj.h"
 
 #include <stdio.h>
@@ -93,6 +94,7 @@ HINSTANCE hinstance;
 static StringOption Arch ("", 'a', "arch", "architecture to install (x86_64 or x86)", false);
 static BoolOption UnattendedOption (false, 'q', "quiet-mode", "Unattended setup mode");
 static BoolOption PackageManagerOption (false, 'M', "package-manager", "Semi-attended chooser-only mode");
+static BoolOption NoAdminOption (false, 'B', "no-admin", "Do not check for and enforce running as Administrator");
 static BoolOption HelpOption (false, 'h', "help", "print help");
 static BOOL WINAPI (*dyn_AttachConsole) (DWORD);
 static BOOL WINAPI (*dyn_GetLongPathName) (LPCTSTR, LPTSTR, DWORD);
@@ -289,6 +291,57 @@ WinMain (HINSTANCE h,
 						<< "\nCommand Line Options:\n");
     else
       {
+	OSVERSIONINFO version;
+	version.dwOSVersionInfoSize = sizeof version;
+	GetVersionEx (&version);
+	if ((version.dwMajorVersion >= 6)
+			&& !NoAdminOption && !nt_sec.isRunAsAdmin ())
+	  {
+		log (LOG_PLAIN) << "Attempting to elevate to Administrator" << endLog;
+		char exe_path[MAX_PATH];
+		if (!GetModuleFileName(NULL, exe_path, ARRAYSIZE(exe_path)))
+		  {
+			log (LOG_TIMESTAMP) << "GetModuleFileName() failed: " << GetLastError () << endLog;
+			goto finish_up;
+		  }
+
+		SHELLEXECUTEINFO sei = { sizeof(sei) };
+		sei.lpVerb = "runas";
+		sei.lpFile = exe_path;
+		sei.nShow = SW_NORMAL;
+
+		// Note, this is necessary to avoid an infinite loop.
+		// The understanding is that pre-Vista, the runas verb will not
+		// result in a privilege elevated process. Therefore we need to
+		// indicate to the forked process that it should be happy with
+		// whatever privileges it is run with.
+		std::string command_line_cs (command_line);
+		command_line_cs += " -";
+		command_line_cs += NoAdminOption.shortOption();
+		sei.lpParameters = command_line_cs.c_str ();
+
+		// avoid the ambiguity of having both the parent and child
+		// process logging simultaneously, by ending it here. perhaps
+		// overkill, but safe.
+		theLog->flushAll ();
+		theLog->clearFiles ();
+
+		if (!ShellExecuteEx(&sei))
+		  {
+			// Note: if user declined, we get an ERROR_CANCELLED.
+			// Merely to be compatible with existing Cygwin Setup
+			// behaviour. we do nothing with it but just exit.
+			// Future improvement is possible here.
+
+			// TODO: because we have been prudent and closed off the
+			// logging, we can't actually log in this way. Though it
+			// would be helpful
+//			log (LOG_TIMESTAMP) << "ShellExecuteEx() failed: " << GetLastError () << endLog;
+		  }
+		// once we are set on a course to privilege elevate, the parent
+		// process is unnecessary
+		goto finish_up;
+	  }
 	UserSettings Settings (local_dir);
 
 	main_display ();
@@ -296,6 +349,7 @@ WinMain (HINSTANCE h,
 	Settings.save ();	// Clean exit.. save user options.
       }
 
+finish_up:
     if (rebootneeded)
       {
 	theLog->exit (IDS_REBOOT_REQUIRED);
diff --git a/setup.exe.manifest b/setup.exe.manifest
index c195ebc..394f19f 100755
--- a/setup.exe.manifest
+++ b/setup.exe.manifest
@@ -19,6 +19,13 @@
 	  />
       </dependentAssembly>
   </dependency>
+  <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
+    <security>
+      <requestedPrivileges>
+        <requestedExecutionLevel level="asInvoker" uiAccess="false"/>
+      </requestedPrivileges>
+    </security>
+  </trustInfo>
   <compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
     <application>
       <!--The ID below indicates application support for Windows Vista -->
diff --git a/setup64.exe.manifest b/setup64.exe.manifest
index 61c5241..f0bf282 100755
--- a/setup64.exe.manifest
+++ b/setup64.exe.manifest
@@ -22,7 +22,7 @@
   <trustInfo xmlns="urn:schemas-microsoft-com:asm.v3">
     <security>
       <requestedPrivileges>
-        <requestedExecutionLevel level="requireAdministrator" uiAccess="false"/>
+        <requestedExecutionLevel level="asInvoker" uiAccess="false"/>
       </requestedPrivileges>
     </security>
   </trustInfo>
diff --git a/win32.cc b/win32.cc
index 31a923d..11e323d 100644
--- a/win32.cc
+++ b/win32.cc
@@ -400,6 +400,18 @@ NTSecurity::setDefaultSecurity ()
     setAdminGroup ();
 }
 
+bool
+NTSecurity::isRunAsAdmin ()
+{
+  BOOL is_run_as_admin = FALSE;
+  if (!CheckTokenMembership(NULL, administratorsSID.theSID (), &is_run_as_admin))
+  {
+	  NoteFailedAPI("CheckTokenMembership(administratorsSID)");
+  }
+  return (is_run_as_admin == TRUE);
+}
+
+
 VersionInfo::VersionInfo ()
 {
   v.dwOSVersionInfoSize = sizeof (OSVERSIONINFO);
diff --git a/win32.h b/win32.h
index 7838dcc..91ff184 100644
--- a/win32.h
+++ b/win32.h
@@ -132,6 +132,7 @@ public:
   void resetPrimaryGroup();
   void setAdminGroup ();
   void setDefaultSecurity();
+  bool isRunAsAdmin ();
 private:
   void NoteFailedAPI (const std::string &);
   bool wellKnownSIDsinitialized () const { return _wellKnownSIDsinitialized; }

Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Thu, Nov 07, 2013 at 11:39:51AM +1100, Shaddy Baddah wrote:
>Hi,
>
>
>On Nov 07 11:23, Christopher Faylor wrote:
>>> Hi,
>>>
>>> As discussed, please find the patch to allow setup to be run as a
>>> non-admin user include clear text.

Thanks.  I'll let Corinna have the final say on this but I really
appreciate your willingness to provide a patch.

cgf

[GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2783 bytes --]

Hi Shaddy,

On Nov  7 11:39, Shaddy Baddah wrote:
> 2013-11-06  Shaddy Baddah <lithium-cygwin at shaddybaddah dot name>
> 
> 	* LogFile.cc (LogFile::flushAll): New function to flush log all logging to
> 	files without exiting (as LogFile::exit does).
> 	* LogFile.h: Declare new method closeAll.
> 	* main.cc (NoAdminOption): Add new CLI options -B/--no-admin. This option
> 	allows the user to suppress privilege elevation (in tandem with
> 	"asInvoker" requestedExecutionLevel changes to exe manifests).
> 	(WinMain): check if setup run with Administrator privilege and if the
> 	NoAdminOption has not been specified, attempt to elevate privilege to an
> 	Administrator via WINAPI ShellExecuteEx().
> 	* setup.exe.manifest: Add requestedExecutionLevel of asInvoker to allow
> 	suppression of privilege elevation.
> 	* setup64.exe.manifest: Modify requestedExecutionLevel from
> 	requireAdministrator to asInvoker to allow suppression of privilege
> 	elevation. Continuity of privilege elevation attempt on startup is
> 	implemented by main.cc changes to WinMain().
> 	* win32.cc (NTSecurity::isRunAsAdmin): New function to allow main.cc to
> 	check if setup.exe has been run with privilege elevated to Administrator
> 	level.
> 	* win32.h: Declare new method isRunAsAdmin.

Thanks a lot for this patch.  I applied it with a few minor tweaks.
First of all, this comment was a bit misleading now, given that the
code doesn't run on pre-Vista anyway:

> +		// Note, this is necessary to avoid an infinite loop.
> +		// The understanding is that pre-Vista, the runas verb will not
> +		// result in a privilege elevated process. Therefore we need to
> +		// indicate to the forked process that it should be happy with
> +		// whatever privileges it is run with.
> +		std::string command_line_cs (command_line);
> +		command_line_cs += " -";
> +		command_line_cs += NoAdminOption.shortOption();
> +		sei.lpParameters = command_line_cs.c_str ();

I shortened the comment to a simple one-liner:

              // Avoid another isRunAsAdmin check in the child.

I also added a small change for the sake of starting setup from the
command line.  While the log to the logfiles has been stopped, the
log to stdout persist up to the call of theLog->exit.  I added a 
bit of code to stop printing

  Ending cygwin install
  
if the elevation was successful.  In that case the stdout log now prints

  note: Hand installation over to elevated child process.


Thanks again for this patch, it's highly appreciated and is worth
a gold star, I think.

Chris, do your worst ;)


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Thu, Nov 07, 2013 at 02:15:21PM +0100, Corinna Vinschen wrote:
>Hi Shaddy,
>
>On Nov  7 11:39, Shaddy Baddah wrote:
>> 2013-11-06  Shaddy Baddah <lithium-cygwin at shaddybaddah dot name>
>> 
>> 	* LogFile.cc (LogFile::flushAll): New function to flush log all logging to
>> 	files without exiting (as LogFile::exit does).
>> 	* LogFile.h: Declare new method closeAll.
>> 	* main.cc (NoAdminOption): Add new CLI options -B/--no-admin. This option
>> 	allows the user to suppress privilege elevation (in tandem with
>> 	"asInvoker" requestedExecutionLevel changes to exe manifests).
>> 	(WinMain): check if setup run with Administrator privilege and if the
>> 	NoAdminOption has not been specified, attempt to elevate privilege to an
>> 	Administrator via WINAPI ShellExecuteEx().
>> 	* setup.exe.manifest: Add requestedExecutionLevel of asInvoker to allow
>> 	suppression of privilege elevation.
>> 	* setup64.exe.manifest: Modify requestedExecutionLevel from
>> 	requireAdministrator to asInvoker to allow suppression of privilege
>> 	elevation. Continuity of privilege elevation attempt on startup is
>> 	implemented by main.cc changes to WinMain().
>> 	* win32.cc (NTSecurity::isRunAsAdmin): New function to allow main.cc to
>> 	check if setup.exe has been run with privilege elevated to Administrator
>> 	level.
>> 	* win32.h: Declare new method isRunAsAdmin.
>
>Thanks a lot for this patch.  I applied it with a few minor tweaks.
>First of all, this comment was a bit misleading now, given that the
>code doesn't run on pre-Vista anyway:
>
>> +		// Note, this is necessary to avoid an infinite loop.
>> +		// The understanding is that pre-Vista, the runas verb will not
>> +		// result in a privilege elevated process. Therefore we need to
>> +		// indicate to the forked process that it should be happy with
>> +		// whatever privileges it is run with.
>> +		std::string command_line_cs (command_line);
>> +		command_line_cs += " -";
>> +		command_line_cs += NoAdminOption.shortOption();
>> +		sei.lpParameters = command_line_cs.c_str ();
>
>I shortened the comment to a simple one-liner:
>
>              // Avoid another isRunAsAdmin check in the child.
>
>I also added a small change for the sake of starting setup from the
>command line.  While the log to the logfiles has been stopped, the
>log to stdout persist up to the call of theLog->exit.  I added a 
>bit of code to stop printing
>
>  Ending cygwin install
>  
>if the elevation was successful.  In that case the stdout log now prints
>
>  note: Hand installation over to elevated child process.
>
>
>Thanks again for this patch, it's highly appreciated and is worth
>a gold star, I think.
>
>Chris, do your worst ;)

The new setup's are installed.  Shaddy, do you want to respond to the
Cygwin ML thread and tell them that you've fixed the problem?

Thanks again for doing this.

cgf

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Andrew Schulman]

> Thanks again for this patch, it's highly appreciated and is worth
> a gold star, I think.

Awarded:  http://cygwin.com/goldstars/#SB

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Shaddy Baddah]

Hi,

On Nov 08 02:23, Christopher Faylor wrote:
> On Thu, Nov 07, 2013 at 02:15:21PM +0100, Corinna Vinschen wrote:
>> Hi Shaddy,
>>
>> On Nov  7 11:39, Shaddy Baddah wrote:
>>> 2013-11-06  Shaddy Baddah <lithium-cygwin at shaddybaddah dot name>
>>>
>>> 	* LogFile.cc (LogFile::flushAll): New function to flush log all logging to
>>> 	files without exiting (as LogFile::exit does).
>>> 	* LogFile.h: Declare new method closeAll.
>>> 	* main.cc (NoAdminOption): Add new CLI options -B/--no-admin. This option
>>> 	allows the user to suppress privilege elevation (in tandem with
>>> 	"asInvoker" requestedExecutionLevel changes to exe manifests).
>>> 	(WinMain): check if setup run with Administrator privilege and if the
>>> 	NoAdminOption has not been specified, attempt to elevate privilege to an
>>> 	Administrator via WINAPI ShellExecuteEx().
>>> 	* setup.exe.manifest: Add requestedExecutionLevel of asInvoker to allow
>>> 	suppression of privilege elevation.
>>> 	* setup64.exe.manifest: Modify requestedExecutionLevel from
>>> 	requireAdministrator to asInvoker to allow suppression of privilege
>>> 	elevation. Continuity of privilege elevation attempt on startup is
>>> 	implemented by main.cc changes to WinMain().
>>> 	* win32.cc (NTSecurity::isRunAsAdmin): New function to allow main.cc to
>>> 	check if setup.exe has been run with privilege elevated to Administrator
>>> 	level.
>>> 	* win32.h: Declare new method isRunAsAdmin.
>>
>> Thanks a lot for this patch.  I applied it with a few minor tweaks.
>> First of all, this comment was a bit misleading now, given that the
>> code doesn't run on pre-Vista anyway:
>>
>>> +		// Note, this is necessary to avoid an infinite loop.
>>> +		// The understanding is that pre-Vista, the runas verb will not
>>> +		// result in a privilege elevated process. Therefore we need to
>>> +		// indicate to the forked process that it should be happy with
>>> +		// whatever privileges it is run with.
>>> +		std::string command_line_cs (command_line);
>>> +		command_line_cs += " -";
>>> +		command_line_cs += NoAdminOption.shortOption();
>>> +		sei.lpParameters = command_line_cs.c_str ();
>>
>> I shortened the comment to a simple one-liner:
>>
>>               // Avoid another isRunAsAdmin check in the child.
>>
>> I also added a small change for the sake of starting setup from the
>> command line.  While the log to the logfiles has been stopped, the
>> log to stdout persist up to the call of theLog->exit.  I added a
>> bit of code to stop printing
>>
>>   Ending cygwin install
>>
>> if the elevation was successful.  In that case the stdout log now prints
>>
>>   note: Hand installation over to elevated child process.
>>
>>
>> Thanks again for this patch, it's highly appreciated and is worth
>> a gold star, I think.
>>
>> Chris, do your worst ;)
>
> The new setup's are installed.  Shaddy, do you want to respond to the
> Cygwin ML thread and tell them that you've fixed the problem?
>
> Thanks again for doing this.

Done. Don't mention it. I'm delighted to give back to the Cygwin
community and thank you all for what I feel is one of the best, if not
the best open source project and community.

I'll stick around too and help if there is any issues with the patch.

I am also hoping to find time in the near future to ITP a couple of
packages.

-- 
Regards,
Shaddy

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Sat, Nov 09, 2013 at 11:04:34AM +1100, Shaddy Baddah wrote:
>On Nov 08 02:23, Christopher Faylor wrote:
>> Thanks again for doing this.
>
>Done. Don't mention it.

I have to mention it because your type of contribution is so rare in
this project.  There are only a handful of people who contribute code so
you're very much appreciated.

>I'm delighted to give back to the Cygwin community and thank you all
>for what I feel is one of the best, if not the best open source project
>and community.
>
>I'll stick around too and help if there is any issues with the patch.

Did you see the comments in the list?  Are they valid complaints?

cgf

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2100 bytes --]

On Nov  8 19:40, Christopher Faylor wrote:
> On Sat, Nov 09, 2013 at 11:04:34AM +1100, Shaddy Baddah wrote:
> >On Nov 08 02:23, Christopher Faylor wrote:
> >> Thanks again for doing this.
> >
> >Done. Don't mention it.
> 
> I have to mention it because your type of contribution is so rare in
> this project.  There are only a handful of people who contribute code so
> you're very much appreciated.
> 
> >I'm delighted to give back to the Cygwin community and thank you all
> >for what I feel is one of the best, if not the best open source project
> >and community.
> >
> >I'll stick around too and help if there is any issues with the patch.
> 
> Did you see the comments in the list?  Are they valid complaints?

Maybe I'm dense but I don't quite understand it.  Under 32 bit, a tool
called "setup-foo" will be recognized as an installer binary.  Therefore
the "helpful" UAC installer recognition will try to start setup-x86 as
an installer with admin permissions, asking for consent (default for
admin accounts) or admin credentials (default for non-admin accounts).

How was it possible at all to start 32 bit setup as normal user, without
getting the elevation prompt?

Or, hmm...

[do you hear me thinking?]

...does the UAC installer recognition only kick in for an UAC crippled
admin account but not for a normal user account?

[...testing...]

I just started an older setup-x86 on Windows 8.1 and Windows 7 using
a non-admin user account, and in both cases I have been asked for
administrator credentials.

Which means, I still don't understand how anybody ran setup from
http://cygwin.com/setup-x86.exe as a normal user account without
being asked for admin creds.

Unless the admins of these machines have switched off the installer
recognition.  In that case non-admins could simply start setup-x86 from
the net and now they can't anymore.  Do we still want to support this?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

RE: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Buchbinder, Barry (NIH/NIAID) [E]]

Corinna Vinschen sent the following at Saturday, November 09, 2013 5:20 AM
>Maybe I'm dense but I don't quite understand it. Under 32 bit, a tool
>called "setup-foo" will be recognized as an installer binary. Therefore
>the "helpful" UAC installer recognition will try to start setup-x86 as
>an installer with admin permissions, asking for consent (default for
>admin accounts) or admin credentials (default for non-admin accounts).
>
>How was it possible at all to start 32 bit setup as normal user, without
>getting the elevation prompt?
>
>Or, hmm...
>
>[do you hear me thinking?]
>
>...does the UAC installer recognition only kick in for an UAC crippled
>admin account but not for a normal user account?
>
>[...testing...]
>
>I just started an older setup-x86 on Windows 8.1 and Windows 7 using
>a non-admin user account, and in both cases I have been asked for
>administrator credentials.
>
>Which means, I still don't understand how anybody ran setup from
>http://cygwin.com/setup-x86.exe as a normal user account without being
>asked for admin creds.
>
>Unless the admins of these machines have switched off the installer
>recognition. In that case non-admins could simply start setup-x86 from
>the net and now they can't anymore. Do we still want to support this?

If support is dropped, the Cygwin home page should explain what
non-admins need to do to install 32 bit Cygwin.

The solution on this list has long been "download and rename to
foo.exe".  That could be done for all by renaming
http://cygwin.com/setup-x86.exe to http://cygwin.com/getcygwin32.exe.

I've often wondered whether there is a reason why this hasn't been
done before.  If there is, I'd be interested in learning why, for
my education.  Is there a benefit to having an installer name that
includes "setup" or "install"?  (If there is, I'll drop the topic.)

This is a suggestion, not a request.  This doesn't affect me because I
almost never run setup from the browser.  I run it off my hard disk,
downloading updates when they are released.

Thanks,

- Barry
  Disclaimer: Statements made herein are not made on behalf of NIAID.

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 3106 bytes --]

On Nov  9 16:43, Buchbinder, Barry (NIH/NIAID) [E] wrote:
> Corinna Vinschen sent the following at Saturday, November 09, 2013 5:20 AM
> >Maybe I'm dense but I don't quite understand it. Under 32 bit, a tool
> >called "setup-foo" will be recognized as an installer binary. Therefore
> >the "helpful" UAC installer recognition will try to start setup-x86 as
> >an installer with admin permissions, asking for consent (default for
> >admin accounts) or admin credentials (default for non-admin accounts).
> >
> >How was it possible at all to start 32 bit setup as normal user, without
> >getting the elevation prompt?
> >
> >Or, hmm...
> >
> >[do you hear me thinking?]
> >
> >...does the UAC installer recognition only kick in for an UAC crippled
> >admin account but not for a normal user account?
> >
> >[...testing...]
> >
> >I just started an older setup-x86 on Windows 8.1 and Windows 7 using
> >a non-admin user account, and in both cases I have been asked for
> >administrator credentials.
> >
> >Which means, I still don't understand how anybody ran setup from
> >http://cygwin.com/setup-x86.exe as a normal user account without being
> >asked for admin creds.
> >
> >Unless the admins of these machines have switched off the installer
> >recognition. In that case non-admins could simply start setup-x86 from
> >the net and now they can't anymore. Do we still want to support this?
> 
> If support is dropped, the Cygwin home page should explain what
> non-admins need to do to install 32 bit Cygwin.
> 
> The solution on this list has long been "download and rename to
> foo.exe".  That could be done for all by renaming
> http://cygwin.com/setup-x86.exe to http://cygwin.com/getcygwin32.exe.
> 
> I've often wondered whether there is a reason why this hasn't been
> done before.  If there is, I'd be interested in learning why, for
> my education.  Is there a benefit to having an installer name that
> includes "setup" or "install"?  (If there is, I'll drop the topic.)

The installation as admin for "all users" was always, and should stay,
the default.  All home main users on XP were admins anyway so this
wasn't a problem before.  Under UAC, the installer recognition handled
that so far, on 64 bit the "asAdmin" manifest.

Shaddy's change makes the setup-xxx name useless since the binary
elevates itself, but in fact this change eliminates the name discussion
entirely.  UAC installer recognition doesn't kick in anymore given the
"asInvoker" mainfest.  It doesn't matter anymore if the binary is called
setup or blurb, it will behave identically.

What changed is the way how normal users can install for "just them".
No name tweak but an option instead.  Given what you wrote, an
installation as normal user right from the net was not possible before,
so just the method to do it changed slightly.  By documenting it
somewhere, we should be all set, shouldn't we?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Sat, Nov 09, 2013 at 06:30:50PM +0100, Corinna Vinschen wrote:
>What changed is the way how normal users can install for "just them".
>No name tweak but an option instead.  Given what you wrote, an
>installation as normal user right from the net was not possible before,
>so just the method to do it changed slightly.  By documenting it
>somewhere, we should be all set, shouldn't we?

So, in other words, an end user will no longer have to rename setup*.exe
to foo.exe to bypass enforced elevation but will, instead, just have to
use a command-line option.  Sounds good to me.  We can add words for
that to the install.html web page and to the FAQ.

cgf

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2627 bytes --]

On Nov 10 02:28, Christopher Faylor wrote:
> On Sat, Nov 09, 2013 at 06:30:50PM +0100, Corinna Vinschen wrote:
> >What changed is the way how normal users can install for "just them".
> >No name tweak but an option instead.  Given what you wrote, an
> >installation as normal user right from the net was not possible before,
> >so just the method to do it changed slightly.  By documenting it
> >somewhere, we should be all set, shouldn't we?
> 
> So, in other words, an end user will no longer have to rename setup*.exe
> to foo.exe to bypass enforced elevation but will, instead, just have to
> use a command-line option.  Sounds good to me.  We can add words for
> that to the install.html web page and to the FAQ.

Exactly what I had in mind.  I have some changes to setup-net.xml in the
loop.  I'll add some more to the FAQ and upload that next week.

Nevertheless, on second thought we *could* do more, if we want to,
now that we have our permissions completely under our own control.

Provided somebody has fun working on that stuff, what we could do,
for instance:

- Per the Microsoft UAC guidelines(*) the elevation prompt should not
  be shown at all if UAC is switched off.  The idea is to show a dialog
  instead, telling the user "this application requires admin privs,
  yada yada", but in fact our setup would run as normal user just fine
  if we let it.  See the next point.

- Right now setup simply exits if the elevation didn't work or was
  canceled.  What about a dialog instead, which asks the user something
  along the lines of "Elevation canceled" or "UAC turned off", and then
  "Setup can run without admin privs with some restrictions, are you
  aware of them and do you want to do that? [Yes/No]"

- This could be even more elegant if setup checks if the installer path
  in the registry is in HKLM.  If so, it could refuse to do its stuff
  without admin rights, because it knows that the original installation
  has been performed with admin rights.  Chances are high then, that a
  normal user won't have enough permissions to update the installation.

- Something we could have done all along (and which has been mentioned
  on the Cygwin ML): We could drop the "All users"/"Just me" choice if
  the user has no admin rights.  After all, the "All user" stuff can't
  be written anyway without admin rights.


Corinna


(*) http://msdn.microsoft.com/en-us/library/windows/desktop/aa511445.aspx

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [GOLDSTAR] Re: [PATCH] setup: allow running as non-admin

[Christopher Faylor]

On Sun, Nov 10, 2013 at 01:28:02PM +0100, Corinna Vinschen wrote:
>On Nov 10 02:28, Christopher Faylor wrote:
>> On Sat, Nov 09, 2013 at 06:30:50PM +0100, Corinna Vinschen wrote:
>> >What changed is the way how normal users can install for "just them".
>> >No name tweak but an option instead.  Given what you wrote, an
>> >installation as normal user right from the net was not possible before,
>> >so just the method to do it changed slightly.  By documenting it
>> >somewhere, we should be all set, shouldn't we?
>> 
>> So, in other words, an end user will no longer have to rename setup*.exe
>> to foo.exe to bypass enforced elevation but will, instead, just have to
>> use a command-line option.  Sounds good to me.  We can add words for
>> that to the install.html web page and to the FAQ.
>
>Exactly what I had in mind.  I have some changes to setup-net.xml in the
>loop.  I'll add some more to the FAQ and upload that next week.
>
>Nevertheless, on second thought we *could* do more, if we want to,
>now that we have our permissions completely under our own control.
>
>Provided somebody has fun working on that stuff, what we could do,
>for instance:
>
>- Per the Microsoft UAC guidelines(*) the elevation prompt should not
>  be shown at all if UAC is switched off.  The idea is to show a dialog
>  instead, telling the user "this application requires admin privs,
>  yada yada", but in fact our setup would run as normal user just fine
>  if we let it.  See the next point.
>
>- Right now setup simply exits if the elevation didn't work or was
>  canceled.  What about a dialog instead, which asks the user something
>  along the lines of "Elevation canceled" or "UAC turned off", and then
>  "Setup can run without admin privs with some restrictions, are you
>  aware of them and do you want to do that? [Yes/No]"
>
>- This could be even more elegant if setup checks if the installer path
>  in the registry is in HKLM.  If so, it could refuse to do its stuff
>  without admin rights, because it knows that the original installation
>  has been performed with admin rights.  Chances are high then, that a
>  normal user won't have enough permissions to update the installation.
>
>- Something we could have done all along (and which has been mentioned
>  on the Cygwin ML): We could drop the "All users"/"Just me" choice if
>  the user has no admin rights.  After all, the "All user" stuff can't
>  be written anyway without admin rights.

All good SHTDI ideas.

cgf