Go Back   nV News Forums > Software Forums > Software Development

Newegg Daily Deals

Reply
 
Thread Tools
Old 05-09-07, 08:45 AM   #1
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default C help

Background
We use SSHFS to dump images of our daughter (18 mo) to her website (my server). uid/guid ownership on the file is sshfs user:user and the uid www-data needs write access to the file so it can be converted/resized using Imagemagick.

The below code runs suid (4770) with root:www-data and is called from a PHP script. The PHP code checks fileowner() on the image and if the owner is not www-data the below C code is called via system().

The data is sanitized in PHP prior to being passed to the suid binary, however, I want additional security measures in place. For example, no content will be outside of /www/dlf/dlf-albums.

Here is an example argv[1] passed value:
/www/dlf/dlf-albums/12-24 Months/Big Bubble bath 1.JPG

Here is the PHP code snippet that calls the suid binary:
Code:
//Fix file permissions if they are invalid, 33 is UID of www-data, fixperms is SUID 4770 root:www-data
if (fileowner(realpath($cache_path.$this->get_files[$i]['real'])) != 33)
  system("/www/dlf/fixperms \"".realpath($cache_path.$this->get_files[$i]['real'])."\"");
Issues
I am not proficient in C by any stretch. I need the code modified to:
1) Check that argv[1] does not traverse outside of /var/www/dlf-albums
2) Use fprintf() instead of fputs() (isn't fputs less secure)?
3) For some reason asctime() is adding a newline character to the end of the string?
4) Be able to print output using something like fputs(fout,"%s\t%s\n",asctime(time(NULL)),argv[1]); without segfaulting
5) Create as secure code as possible.

I certainly appreciate any help someone can give me. Again, C is not my strong point by any stretch.

Code:
/* evilghost
 * May 8, 2007
 * SETUID binary to fix permissions with uploaded images if they are invalid.
 * Permissions on compiled binary must be 4770 with uid/gid root:www-data
 */

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>

#define wwwdata_GID 33
#define shared_GID 5140

FILE *fout;

int main(int argc, char **argv){
        time_t curtime;
        struct tm *loctime;

        // Get the current time & convert to local.
        curtime = time (NULL);
        loctime = localtime (&curtime);

        //Write audit trail.
        fout = fopen ("/www/dlf/fixperms.log", "a+");
        fputs(argv[1],fout);
        fputs("\t",fout);
        fputs(asctime(loctime),fout);
        fclose(fout);

        //Adjust permissions
        chown(argv[1], wwwdata_GID, shared_GID);
        chmod(argv[1], 0770);
        return(0);
}
evilghost is offline   Reply With Quote
Old 05-09-07, 10:20 AM   #2
wnd
Nerd, Geek, Freak
 
wnd's Avatar
 
Join Date: Sep 2005
Location: Finland
Posts: 703
Default Re: C help

Quote:
Originally Posted by evilghost
Here is an example argv[1] passed value:
/www/dlf/dlf-albums/12-24 Months/Big Bubble bath 1.JPG

1) Check that argv[1] does not traverse outside of /var/www/dlf-albums
2) Use fprintf() instead of fputs() (isn't fputs less secure)?
3) For some reason asctime() is adding a newline character to the end of the string?
4) Be able to print output using something like fputs(fout,"%s\t%s\n",asctime(time(NULL)),argv[1]); without segfaulting
5) Create as secure code as possible.
1. If you only want to make sure only legimate paths pass through, you could simply make sure the string begins with your basepath, and no "/.." exists in the path. Maybe something in spirit of...
Code:
if (argc != 2 || argv[1] == NULL
                || strncmp(argv[1], BASEPATH, strlen(BASEPATH)) != 0
                || strstr(argv[1], "/..") != NULL) {
        return EXIT_FAILURE;
}
You will also have to make sure the target file is not a symlink. stat(2) and S_ISLNK(2) should help here.

2. Both will overflow if input string is not terminated. fsnprintf help a bit, but strings pointed by *argv are always terminated unless something is seriously broken. Always pass user-supplied string "through %s" (i.e. printf("%s", input); ), even when unnecessary. Sooner or later you forget to do it when it really counts.

3. The reason is described in the manual. :-) Use strftime if this bothers you.

4.
Code:
char t[80];
t[79] = '\0';
/* checking return value of strftime isn't necessary since t is guaranteed
   to be terminated */
strftime(t, 79, "%a %b %H:%M:%S %Y", loctime);
fprintf(fout, "%s\t%s\n", t, file);
5. Return values. Namely, check return value of fopen and avoid potential segfault.

Maybe you could consider writing the log with syslog(3).
__________________
web | cat

Christianity, noun: The belief that a cosmic Jewish Zombie who was his own father can make you live forever if you symbolically eat his flesh and telepathically tell him you accept him as your master, so he can remove an evil force from your soul that is present in humanity because a rib-woman was convinced by a talking snake to eat from a magical tree. [mad.frog]
wnd is offline   Reply With Quote
Old 05-09-07, 11:22 AM   #3
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

This is good, thank you very much for your time/help.
evilghost is offline   Reply With Quote
Old 05-09-07, 11:38 AM   #4
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

This is what I ended up with, thanks again for your help, feel free to make any additional comments:

Code:
/* evilghost
 * Thanks wnd for code snippets & help.
 * May 8, 2007
 * SETUID binary to fix permissions with uploaded images if they are invalid.
 * Permissions on compiled binary must be 4770 with uid/gid root:www-data
 */

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>

#define wwwdata_GID 33
#define shared_GID 5140
#define BASEPATH "/www/dlf/dlf-albums/"

FILE *fout;

int main(int argc, char **argv){
        time_t curtime;
        struct tm *loctime;
        char t[80];
        t[79] = '\0';

        //Check initial input.
        if (argc != 2 || argv[1] == NULL || strncmp(argv[1], BASEPATH, strlen(BASEPATH)) != 0 || strstr(argv[1], "/..") != NULL) {
                fprintf(stderr,"%s\n","Invalid file path passed.");
                return EXIT_FAILURE;
        }

        // Get the current time & convert to local.
        curtime = time (NULL);
        loctime = localtime (&curtime);
        strftime(t, 79, "%a %b %H:%M:%S %Y", loctime);

        //Write audit trail.
        if ((fout = fopen ("/www/dlf/fixperms.log", "a+"))  != (FILE *)0){
                fprintf(fout, "%s\t%s\n", t, argv[1]);
                fclose(fout);
        }else{
                fprintf(stderr,"%s\n","Error while opening audit log for writing");
                return EXIT_FAILURE;
        }

        //Adjust permissions
        chown(argv[1], wwwdata_GID, shared_GID);
        chmod(argv[1], 0770);
        return(0);
}
evilghost is offline   Reply With Quote
Old 05-09-07, 12:37 PM   #5
wnd
Nerd, Geek, Freak
 
wnd's Avatar
 
Join Date: Sep 2005
Location: Finland
Posts: 703
Default Re: C help

Quote:
Originally Posted by evilghost
This is what I ended up with, thanks again for your help, feel free to make any additional comments
Just some minor things. I tend to be a little picky about (C-)code.

- There is no reason for FILE *fout to be global. It's sufficient to have it only inside main-function.

- Having an assignment, a function call, and a comparision in a single "if (foo)" is very C-ish. However in my opinnion this feature is often abused since complex statements easily become difficult to read.
Code:
fout = fopen("foo", "a+");
if (fout == NULL) {
        fprintf(stderr, "oops\n");
        return EXIT_FAILURE;
}
/* Code continues outside if-block. That's where we want to go. */
- You can compare return value of fopen against NULL.

- It is perfectly safe (as far as printf can be) to have just fprintf(stderr, "Error while...\n");. When I said user input, I meant input from "unregulated" source such as command prompt, file, network, or such. A static string in source code is not user input. :-)

- Similary to EXIT_FAILURE, most stdlib.hs come with EXIT_SUCCESS.

- Since the executable is suid, I'd be extra careful to make sure target file is exactly what it's supposed to be (note that I don't know if uid/gid is set to something specific so that part may not apply):
Code:
#include <sys/types.h>
#include <sys/stat.h>
#include <uninstd.h>

struct stat s;
int r;

r = lstat(file, &s);
if (r != 0) {
        oops;
}
if (! (S_ISREG(s.st_mode) && s.st_uid == OWNER_UID
                        && s.st_gid == OWNER_GID)) {
        oops;
}
/* good to go */
__________________
web | cat

Christianity, noun: The belief that a cosmic Jewish Zombie who was his own father can make you live forever if you symbolically eat his flesh and telepathically tell him you accept him as your master, so he can remove an evil force from your soul that is present in humanity because a rib-woman was convinced by a talking snake to eat from a magical tree. [mad.frog]
wnd is offline   Reply With Quote
Old 05-10-07, 03:01 PM   #6
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

wnd, if you have the time to review the code again I'd certainly appreciate. I don't trust www touching a suid binary so I'm trying to create as much security as possible and you've been a huge help.

I read that using lstat() versus access() causes greater overhead and I should use access() in place with R_OK to test if the file is ready for reading (exists). I don't need to check uid/gid since they can be different values and I'm not too concerned about owner since they are dropped in by authenticated users (me or my wife) over SSH.

I also added a input length check to ensure the path passed doesn't exceed 255 characters, a number I determined to be a reasonable input length.

I also modified the code to take into account your suggestions.

Code:
/* evilghost
 * Thanks wnd for code snippets & help.
 * May 10, 2007
 * SETUID binary to fix permissions with uploaded images if they are invalid.
 * Permissions on compiled binary must be 4770 with uid/gid root:www-data
 */

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>

#define wwwdata_GID 33
#define shared_GID 5140
#define BASEPATH "/www/dlf/dlf-albums/"

int main(int argc, char **argv){

        FILE *fout;
        time_t curtime;
        struct tm *loctime;
        char t[80];
        t[79] = '\0';

        //Check initial input.
        if (argc != 2 || argv[1] == NULL || strncmp(argv[1], BASEPATH, strlen(BASEPATH)) != 0 || strstr(argv[1], "/..") != NULL) {
                fprintf(stderr,"Failed:  Invalid file path passed.\n");
                return EXIT_FAILURE;
        }

        //Check for possible abnormal behavior (excessively long parameter)
        if (strlen(argv[1]) > 255){
                fprintf(stderr,"Failed:  Extraordinarily long file path passed, ignoring.\n");
                return EXIT_FAILURE;
        }

        //Check if file exists and adjust permissions
        if (access(argv[1],R_OK) == 0){
                // Get the current time & convert to local.
                curtime = time (NULL);
                loctime = localtime (&curtime);
                strftime(t, 79, "%a %b %H:%M:%S %Y", loctime);

                //Write audit trail.
                fout = fopen ("/www/dlf/fixperms.log", "a+");
                if (fout != NULL){
                        fprintf(fout, "%s\t%s\n", t, argv[1]);
                        fclose(fout);
                }else{
                        fprintf(stderr,"Failed:  Error while opening audit log for writing\n");
                        return EXIT_FAILURE;
                }

                //Adjust file permissions
                chown(argv[1], wwwdata_GID, shared_GID);
                chmod(argv[1], 0770);

                fprintf(stderr,"Success\n");
                return EXIT_SUCCESS;
        }else{
                fprintf(stderr,"Failed:  File path specified does not exist, no action performed.\n");
                return EXIT_FAILURE;
        }
}
evilghost is offline   Reply With Quote
Old 05-10-07, 06:19 PM   #7
wnd
Nerd, Geek, Freak
 
wnd's Avatar
 
Join Date: Sep 2005
Location: Finland
Posts: 703
Default Re: C help

Quote:
Originally Posted by evilghost
I read that using lstat() versus access() causes greater overhead and I should use access() in place with R_OK to test if the file is ready for reading (exists). I don't need to check uid/gid since they can be different values and I'm not too concerned about owner since they are dropped in by authenticated users (me or my wife) over SSH.
Honestly, I don't know about lstat/access, but I can't see much difference. Both need to read the directory entry and I think it's reasonable to expect all metadata (per file) to be found in a single place, accessed with a single read.

What I do know is that access() happily follows symlinks. In other words you can create a symlink /www/dlf/dlf-albums/foo pointing at /bin and run your suid-executable to give wwwdata_GID (I hope that's a typo and it should read wwwdata_UID instead) full write access to /bin. As result, wwwdata_GID could potentially issue "rm -rf /bin" at his discretion.

Quote:
Originally Posted by evilghost
I also added a input length check to ensure the path passed doesn't exceed 255 characters, a number I determined to be a reasonable input length.
I can't see why this would be necessary (unless chmod(2) and chown(2) are broken), but with such a cheap sanity check you can't go wrong. Thinking of effects of long filenames in the log, I have also begun to wonder why you're not logging failed attempts.

Quote:
Originally Posted by evilghost
I also modified the code to take into account your suggestions.
- Include string.h for strncmp(), and sys/types.h and sys/stat.h for chmod()/chown(). :-)
- wwwdata_GID is probably supposed to be wwwdata_UID.
- If you end up using lstat, consider settings directories to 0770 and files to 0660.

I'm also wondering is suid-executable is the right way in the first place. How about an inbox for the files and a cronjob to set the permissions and move the files into place? How about an sh-script that copies the files in place and sets the files 0660 and directories 0770? How about an sh-script that uploads the files for a cgi-script to process? (for file in $files; do wget --post-file $file https://intra/foo/import?file=foo%20bar/baz.jpeg; done)
__________________
web | cat

Christianity, noun: The belief that a cosmic Jewish Zombie who was his own father can make you live forever if you symbolically eat his flesh and telepathically tell him you accept him as your master, so he can remove an evil force from your soul that is present in humanity because a rib-woman was convinced by a talking snake to eat from a magical tree. [mad.frog]

Last edited by wnd; 05-11-07 at 11:24 AM. Reason: --post-file
wnd is offline   Reply With Quote
Old 05-11-07, 10:47 AM   #8
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

Thanks for your help, I've updated the code and I no longer get compiler warnings, everything looks good. I'm using stat now to check if it's a regular file to ensure symlinks aren't processed.
evilghost is offline   Reply With Quote

Old 05-11-07, 11:23 AM   #9
wnd
Nerd, Geek, Freak
 
wnd's Avatar
 
Join Date: Sep 2005
Location: Finland
Posts: 703
Default Re: C help

Quote:
Originally Posted by evilghost
I'm using stat now to check if it's a regular file to ensure symlinks aren't processed.
I hope you meant lstat. "lstat() is identical to stat(), except that if path is a symbolic link, then the link itself is stat-ed, not the file that it refers to."
__________________
web | cat

Christianity, noun: The belief that a cosmic Jewish Zombie who was his own father can make you live forever if you symbolically eat his flesh and telepathically tell him you accept him as your master, so he can remove an evil force from your soul that is present in humanity because a rib-woman was convinced by a talking snake to eat from a magical tree. [mad.frog]
wnd is offline   Reply With Quote
Old 05-11-07, 01:36 PM   #10
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

I meant lstat, I used your code (thanks). I can repost what I'm using now if you want to take a look, I certainly appreciate your help.
evilghost is offline   Reply With Quote
Old 05-14-07, 04:43 AM   #11
wnd
Nerd, Geek, Freak
 
wnd's Avatar
 
Join Date: Sep 2005
Location: Finland
Posts: 703
Default Re: C help

I think there's very little left to comment but sure, I'll have a look if you paste it here once more. Email and IRC will also do. :-)
__________________
web | cat

Christianity, noun: The belief that a cosmic Jewish Zombie who was his own father can make you live forever if you symbolically eat his flesh and telepathically tell him you accept him as your master, so he can remove an evil force from your soul that is present in humanity because a rib-woman was convinced by a talking snake to eat from a magical tree. [mad.frog]
wnd is offline   Reply With Quote
Old 05-14-07, 08:10 AM   #12
evilghost
Registered User
 
Join Date: Jul 2005
Posts: 3,606
Default Re: C help

Code:
/* evilghost
 * Thanks wnd for code snippets & help.
 * May 10, 2007
 * SETUID binary to fix permissions with uploaded images if they are invalid.
 * Permissions on compiled binary must be 4770 with uid/gid root:www-data
 */

#include <stdlib.h>
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <time.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>

#define wwwdata_UID 33
#define shared_GID 5140
#define BASEPATH "/www/dlf/dlf-albums/"

int main(int argc, char **argv){

        FILE *fout;
        time_t curtime;
        struct tm *loctime;
        char t[80];
        t[79] = '\0';
        struct stat s;
        int r;

        //Check initial input.
        if (argc != 2 || argv[1] == NULL || strncmp(argv[1], BASEPATH, strlen(BASEPATH)) != 0 || strstr(argv[1], "/..") != NULL) {
                fprintf(stderr,"Failed:  Invalid file path passed.\n");
                return EXIT_FAILURE;
        }

        //Check for possible abnormal behavior (excessively long parameter)
        if (strlen(argv[1]) > 255){
                fprintf(stderr,"Failed:  Extraordinarily long file path passed, ignoring.\n");
                return EXIT_FAILURE;
        }

        //Check if file exists and adjust permissions
        if (access(argv[1],R_OK) == 0){
                //Check if it's a regular file.
                r = lstat(argv[1], &s);
                if (r != 0){
                        fprintf(stderr,"Failed: Unable to stat file path\n");
                        return EXIT_FAILURE;
                }

                if (! (S_ISREG(s.st_mode))){
                        fprintf(stderr,"Failed:  File path is not a regular file\n");
                        return EXIT_FAILURE;
                }

                // Get the current time & convert to local.
                curtime = time (NULL);
                loctime = localtime (&curtime);
                strftime(t, 79, "%T %D", loctime);

                //Write audit trail.
                fout = fopen ("/www/dlf/fixperms.log", "a+");
                if (fout != NULL){
                        fprintf(fout, "%s\t%s\n", t, argv[1]);
                        fclose(fout);
                }else{
                        fprintf(stderr,"Failed:  Error while opening audit log for writing\n");
                        return EXIT_FAILURE;
                }

                //Adjust file permissions
                chown(argv[1], wwwdata_UID, shared_GID);
                chmod(argv[1], 0770);

                fprintf(stderr,"Success\n");
                return EXIT_SUCCESS;
        }else{
                fprintf(stderr,"Failed:  File path specified does not exist, no action performed.\n");
                return EXIT_FAILURE;
        }
}
evilghost is offline   Reply With Quote
Reply


Thread Tools

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off
Forum Jump


All times are GMT -5. The time now is 12:51 PM.


Powered by vBulletin® Version 3.7.1
Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Copyright 1998 - 2014, nV News.