nV News Forums
Page 1 of 2 1 2 >
Show 40 post(s) from this thread on one page

 
 

nV News Forums (http://www.nvnews.net/vbulletin/index.php)
-   Software Development (http://www.nvnews.net/vbulletin/forumdisplay.php?f=53)
-   -   C help (http://www.nvnews.net/vbulletin/showthread.php?t=91169)

evilghost 05-09-07 08:45 AM
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);
}

wnd 05-09-07 10:20 AM
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).

evilghost 05-09-07 11:22 AM
Re: C help
 
This is good, thank you very much for your time/help.

evilghost 05-09-07 11:38 AM
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);
}

wnd 05-09-07 12:37 PM
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 */

evilghost 05-10-07 03:01 PM
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;
        }
}

wnd 05-10-07 06:19 PM
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)

evilghost 05-11-07 10:47 AM
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.

wnd 05-11-07 11:23 AM
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."

evilghost 05-11-07 01:36 PM
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.

wnd 05-14-07 04:43 AM
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. :-)

evilghost 05-14-07 08:10 AM
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;
        }
}


All times are GMT -5. The time now is 05:12 AM.
Page 1 of 2 1 2 >
Show 40 post(s) from this thread on one page

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