From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 1/2] misc-progs: addonctrl: Add support for 'Services' metadata Date: Tue, 04 Oct 2022 11:28:58 +0100 Message-ID: <9C06AF20-31D0-4CEE-9779-2E4E9B1C64BC@ipfire.org> In-Reply-To: <20221003152720.13140-3-robin.roevens@disroot.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0382553591277405763==" List-Id: --===============0382553591277405763== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello Robin, This is a little bit harder to review... > On 3 Oct 2022, at 16:27, Robin Roevens wrote: >=20 > * Addonctrl will now check in addon metadata for the exact initscript > names (Services). If more than one initscript is defined for an addon, > the requested action will be performed on all listed initscripts. > * Added posibility to perform action on a specific initscript of an > addon instead of on all initscripts of the addon. Is this actually used anywhere? I cannot come up with an example where this w= ould be relevant. > * New action 'list-services' to display a list of services related to > an addon. Agreed. > * New action 'boot-status' to display wether service(s) are enabled > to start on boot or not. Agreed. > * More error checking and cleaner error reporting to user I like this :) > * General cleanup and code restructuring to avoid code duplication > * Updated and made usage instructions more verbose. >=20 > Fixes: Bug#12935 > Signed-off-by: Robin Roevens > --- > src/misc-progs/addonctrl.c | 384 +++++++++++++++++++++++++++++++------ > 1 file changed, 323 insertions(+), 61 deletions(-) >=20 > diff --git a/src/misc-progs/addonctrl.c b/src/misc-progs/addonctrl.c > index 14b4b1325..277bd1809 100644 > --- a/src/misc-progs/addonctrl.c > +++ b/src/misc-progs/addonctrl.c > @@ -10,71 +10,333 @@ > #include > #include > #include > +#include > #include > +#include > +#include > #include "setuid.h" >=20 > #define BUFFER_SIZE 1024 >=20 > +const char *enabled_path =3D "/etc/rc.d/rc3.d"; > +const char *disabled_path =3D "/etc/rc.d/rc3.d/off"; > + > +char errormsg[BUFFER_SIZE] =3D ""; > +char *usage =3D=20 > + "Usage\n" > + " addonctrl (start|stop|restart|reload|enable|disable|status|= boot-status|list-services) []\n" > + "\n" > + "Options:\n" > + " \t\tName of the addon to control\n" > + " \t\tSpecific service of the addon to control (optional)\n" > + " \t\t\tBy default the requested action is performed on all related s= ervices. See also 'list-services'.\n" > + " start\t\t\tStart service(s) of the addon\n" > + " stop\t\t\tStop service(s) of the addon\n" > + " restart\t\tRestart service(s) of the addon\n" > + " enable\t\tEnable service(s) of the addon to start at boot\n" > + " disable\t\tDisable service(s) of the addon to start at boot\n" > + " status\t\tDisplay current state of addon service(s)\n" > + " boot-status\t\tDisplay wether service(s) is enabled on boot or not\= n" > + " list-services\t\tDisplay a list of services related to the addon"; This string could be const, but I am sure the compiler would assume that anyw= ays. > + > +// Check if matches . Wildcard '?' matches any single char= acter. > +// Returns 1 when found, 0 when not found > +int match(const char *pattern, const char *text) { > + if (pattern[0] =3D=3D '\0' && *text =3D=3D '\0') > + return 1; Do you not want to check whether you have reached the end of either string? I would have written it as: if (!*pattern || !*text) return 1; > + if (*text !=3D '\0' && (pattern[0]=3D=3D'?' || pattern[0]=3D=3D*text)) > + return match(pattern+1, text+1); > + return 0; I have nothing against a recursive implementation, but potentially, a for loo= p would have been easier to read: for (unsigned int i =3D 0; i < strlen(text); i++) { // ? matches anything if (pattern[i] =3D=3D '?') continue; // Check for mismatch if (pattern[i] !=3D text[i]) return 1; } return 0; This example does not include checking whether pattern is at least as long as= text. Since this function is only being used here, you could have declared it as st= atic. The compiler might do this for you. > +} > + > +// Find a file using allowing the '?' wildcard. > +// Returns the found filename or NULL if not found > +char* find_file_in_dir(const char *path, const char *filepattern)=20 > +{ > + static struct dirent *entry; > + DIR *dp; > + int found =3D 0; > + > + if ((dp =3D opendir(path)) !=3D NULL) { > + while(found =3D=3D 0 && (entry =3D readdir(dp)) !=3D NULL) > + found =3D match(filepattern, entry->d_name); > + Now, reading through this, I understand what the match function is used for :) There is already a function for this: https://man7.org/linux/man-pages/man3/fnmatch.3.html I think it works exactly the way you want this. > + closedir(dp); > + } > + > + if (! found) { > + return NULL; > + } > + > + return entry->d_name; I am not sure where you can rely on closedir() not deallocating the entry. It would be best to change found to =E2=80=9Cchar *=E2=80=9D and call strdup(= entry->d_name) after you found a match. You can then simply return found at the end which should have been initialise= d to NULL. > +} > + > +// Reads Services metadata for . > +// Returns pointer to array of strings containing the services for = =20 > +// and sets to the number of found services > +char **get_addon_services(const char *addon, int *servicescnt) { > + const char *metafile_prefix =3D "/opt/pakfire/db/installed/meta-"; > + const char *metadata_key =3D "Services"; > + const char *keyvalue_delim =3D ":"; > + const char *service_delim =3D " "; > + char *token; > + char **services =3D NULL; > + char *service; > + char line[BUFFER_SIZE]; > + int i =3D 0; Since you are passing =E2=80=9Caddon=E2=80=9D to strlen(), I wouldn=E2=80=99t= mind checking that it isn=E2=80=99t NULL, like so: if (!addon) { errno =3D EINVAL; return NULL; } > + char *metafile =3D malloc((strlen(metafile_prefix) + strlen(addon) + 1= ) * sizeof(char)); Why do you multiply this by the size of char? malloc() already takes bytes. You should also check whether you could successfully allocate the memory and = if not, return NULL. > +=20 > + sprintf(metafile, "%s%s", metafile_prefix, addon); Alternatively, you can use asprintf() which will automatically allocate a buf= fer of the correct size. > + FILE *fp =3D fopen(metafile,"r"); > + if ( fp ) { > + // Get initscript(s) for addon from meta-file > + while (!feof(fp) && services =3D=3D NULL) { > + if (fgets(line, BUFFER_SIZE - 1, fp) !=3D NULL) { > + // Strip newline > + char *newline =3D strchr(line, '\n'); > + if (newline) *newline =3D 0; There is a getline() function which would combine this whole block starting f= rom fgets(): https://man7.org/linux/man-pages/man3/getline.3.html getline() will also automatically allocate a buffer of the correct size for e= ach line. > + > + // Parse key/value and look for required key. > + token =3D strtok(line, keyvalue_delim); > + if (token !=3D NULL && strcmp(token, metadata_key) =3D=3D = 0) { > + token =3D strtok(NULL, keyvalue_delim); > + if (token !=3D NULL) { > + services =3D malloc((strlen(token) + 1) * sizeof (= char *)); Same as above. The multiplication is not required I believe. > + // Put each service in services array > + service =3D strtok(token, service_delim); > + while (service !=3D NULL) { > + services[i] =3D malloc((strlen(service) + 1) *= sizeof (char)); > + strcpy(services[i++], service); Those two lines could just be replaced by using strdup(). > + > + service =3D strtok(NULL, service_delim); > + } I am not sure how you are allocating services. It is kind of like a worst-cas= e scenario, that you are doing here. I can live with this, because this is a = very short-lived program and wasting a couple of extra bytes of memory is not= going to be a disaster. But you could also use reallocarray() in the inner while loop and only alloca= te as much memory as you actually need: https://man.archlinux.org/man/reallocarray.3bsd.en > + } > + } > + } else { > + snprintf(errormsg, BUFFER_SIZE - 1, "Could not read '%s' m= etadata for addon '%s'.", metadata_key, addon); > + } > + } > + fclose(fp); > + } else { > + snprintf(errormsg, BUFFER_SIZE - 1, "Addon '%s' not found.\n\n%s",= addon, usage); > + } > + > + free (metafile); > + *servicescnt =3D i; > + return services; > +} > + > +// Calls initscript with parameter > +int initscript_action(const char *service, const char *action) { > + const char *initd_path =3D "/etc/rc.d/init.d"; > + char *command =3D malloc((strlen(initd_path) + 1 + strlen(service) + 1= ) * sizeof(char)); > + int r =3D 0; This function is opening us up for injecting any command from the package met= adata and run it as root. If =E2=80=9Cservice=E2=80=9D for example is =E2=80=9Csamba; reboot=E2=80=9D, = then the initscript that you would want to launch is actually launched first = and then the following reboot command. This cannot stay that way. You will need to use run() from setuid.c which is not launching a shell and i= s therefore making this safer. run() will take the initscript as first argument and any other parameters as = an array. So it could look like this: int initscript_action(const char *service, const char *action) { char buffer[PATH_MAX]; int r; r =3D snprintf(buffer, sizeof(buffer), "/etc/rc.d/init.d/%s", service); if (r < 0) return r; const char* argv[] =3D { action, NULL }; return run(buffer, argv); } I know that you are checking inputs further down below, but I do not want to = take the risk that something (even after some more changes to this file) is s= lipping through making us vulnerable that severely. > + > + sprintf(command, "%s/%s %s", initd_path, service, action); > + r =3D safe_system(command); > + free(command); > + > + return r;=20 > +} > + > +// Move an initscript with filepattern from to > +// Returns: > +// -1: Error during move. Details in errno (returned by C rename functio= n) > +// 0: Success > +// 1: file was not moved, but is already in > +// 2: file does not exist in either in or > +int move_initscript_by_pattern(const char *src_path, const char *dest_path= , const char *filepattern) { > + char *src, *dest; > + int r =3D 1; > + char *filename =3D find_file_in_dir(src_path, filepattern); > + > + if ( filename !=3D NULL ) { > + src =3D malloc((strlen(src_path) + 1 + strlen(filename) + 1) * siz= eof(char)); > + dest =3D malloc((strlen(dest_path) + 1 + strlen(filename) + 1) * s= izeof(char)); > + sprintf(src, "%s/%s", src_path, filename); > + sprintf(dest, "%s/%s", dest_path, filename); See my example above how to allocate a string (or actually not do that at all= by using a stack variable). If allocation fails for some reason, your code will try to write to a NULL po= inter and later free a NULL pointer. > + > + r =3D rename(src, dest); > + > + free(src); > + free(dest); > + } else { > + filename =3D find_file_in_dir(dest_path, filepattern); > + if (filename =3D=3D NULL) > + r =3D 2; > + } > + > + return r; > +} > + > +// Enable/Disable addon service(s) by moving initscript symlink from/to di= sabled_path > +int toggle_service(const char *service, const char *action) { > + const char *src_path, *dest_path; > + char *filepattern =3D malloc((3 + strlen(service) + 1) * sizeof(char)); > + int r =3D 0; > + =20 > + sprintf(filepattern, "S??%s", service); See above. > + > + if (strcmp(action, "enable") =3D=3D 0) { > + src_path =3D disabled_path; > + dest_path =3D enabled_path; > + } else { > + src_path =3D enabled_path; > + dest_path =3D disabled_path; > + } > + > + // Ensure disabled_path exists > + if (mkdir(disabled_path, S_IRWXU + S_IRGRP + S_IXGRP + S_IROTH + S_IXO= TH) =3D=3D -1 && errno !=3D EEXIST) { > + r =3D 1; > + snprintf(errormsg, BUFFER_SIZE -1, "Error creating %s. (Error: %d)= ", disabled_path, errno); > + } else { > + r =3D move_initscript_by_pattern(src_path, dest_path, filepattern); > + if (r =3D=3D -1 ) { > + r =3D 1; > + snprintf(errormsg, BUFFER_SIZE - 1, "Could not %s %s. (Error: = %d)", action, service, errno); > + } else if (r =3D=3D 1) { > + snprintf(errormsg, BUFFER_SIZE - 1, "Service %s is already %sd= . Skipping...", service, action); > + } else if (r =3D=3D 2) { > + snprintf(errormsg, BUFFER_SIZE - 1, "Unable to %s service %s. = (Service has no valid symlink in %s).", action, service, src_path); > + } > + } > + > + free(filepattern); > + =20 > + return r; > +} > + > +// Print to stdout wether is enabled or disabled on boot > +// Prints as Not available when initscript is not found > +// in either enabled_path or disabled_path. > +void print_boot_status(char *service) { > + char *filepattern =3D malloc((3 + strlen(service) + 1) * sizeof(char)); > + sprintf(filepattern, "S??%s", service); See above. > + char *enabled =3D find_file_in_dir(enabled_path, filepattern); > + char *disabled =3D find_file_in_dir(disabled_path, filepattern); > + > + if (enabled !=3D NULL)=20 > + fprintf(stdout, "%s is enabled on boot.\n", service); > + else if (disabled !=3D NULL)=20 > + fprintf(stdout, "%s is disabled on boot.\n", service); > + else > + fprintf(stdout, "%s is not available for boot. (Service has no val= id symlink in either %s or %s).\n", service, enabled_path, disabled_path); > + > + free(filepattern); > +} > + > int main(int argc, char *argv[]) { > - char command[BUFFER_SIZE]; > - > - if (!(initsetuid())) > - exit(1); > - > - if (argc < 3) { > - fprintf(stderr, "\nMissing arguments.\n\naddonctrl addon (start|stop|res= tart|reload|enable|disable)\n\n"); > - exit(1); > - } > - > - const char* name =3D argv[1]; > - > - if (strlen(name) > 32) { > - fprintf(stderr, "\nString to large.\n\naddonctrl addon (start|stop|re= start|reload|enable|disable)\n\n"); > - exit(1); > - } > - > - // Check if the input argument is valid > - if (!is_valid_argument_alnum(name)) { > - fprintf(stderr, "Invalid add-on name: %s\n", name); > - exit(2); > - } > - > - sprintf(command, "/opt/pakfire/db/installed/meta-%s", name); > - FILE *fp =3D fopen(command,"r"); > - if ( fp ) { > - fclose(fp); > - } else { > - fprintf(stderr, "\nAddon '%s' not found.\n\naddonctrl addon (start|st= op|restart|reload|status|enable|disable)\n\n", name); > - exit(1); > - } > - > - if (strcmp(argv[2], "start") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s start", name); > - safe_system(command); > - } else if (strcmp(argv[2], "stop") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s stop", name); > - safe_system(command); > - } else if (strcmp(argv[2], "restart") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s restart", name); > - safe_system(command); > - } else if (strcmp(argv[2], "reload") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s reload", name); > - safe_system(command); > - } else if (strcmp(argv[2], "status") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "/etc/rc.d/init.d/%s status", name); > - safe_system(command); > - } else if (strcmp(argv[2], "enable") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/off/S??%s /etc= /rc.d/rc3.d" , name); > - safe_system(command); > - } else if (strcmp(argv[2], "disable") =3D=3D 0) { > - snprintf(command, BUFFER_SIZE - 1, "mkdir -p /etc/rc.d/rc3.d/off"); > - safe_system(command); > - snprintf(command, BUFFER_SIZE - 1, "mv -f /etc/rc.d/rc3.d/S??%s /etc/rc.= d/rc3.d/off" , name); > - safe_system(command); > - } else { > - fprintf(stderr, "\nBad argument given.\n\naddonctrl addon (start|stop|re= start|reload|enable|disable)\n\n"); > - exit(1); > - } > - > - return 0; > + char **services =3D NULL; > + char **services_ptr =3D NULL; > + int servicescnt =3D 0; > + char *addon =3D argv[1]; > + char *action =3D argv[2]; > + char *service_filter =3D NULL; > + int actioned =3D 0; > + int r =3D 0; > + > + if (!(initsetuid())) > + exit(1); > + > + if (argc < 3) { > + fprintf(stderr, "\nMissing arguments.\n\n%s\n\n", usage); > + exit(1); > + } > + > + if (argc =3D=3D 4)=20 > + service_filter =3D argv[3]; > + > + if (strlen(addon) > 32) { > + fprintf(stderr, "\nString to large.\n\n%s\n\n", usage); > + exit(1); > + } How is 32 chosen? > + > + // Check if the input argument is valid > + if (!is_valid_argument_alnum(addon)) { > + fprintf(stderr, "Invalid add-on name: %s.\n", addon); > + exit(2); > + } > + > + // Get initscript name(s) from addon metadata > + services_ptr =3D get_addon_services(addon, &servicescnt); > + services =3D services_ptr; > + if (services =3D=3D NULL || *services =3D=3D 0) { > + if (strcmp(errormsg, "") !=3D 0) > + fprintf(stderr, "\n%s\n\n", errormsg); > + else > + fprintf(stderr, "\nAddon '%s' has no services.\n\n", addon); > + exit(1); > + } > + > + if (strcmp(action, "start") =3D=3D 0 || > + strcmp(action, "stop") =3D=3D 0 || > + strcmp(action, "restart") =3D=3D 0 || > + strcmp(action, "reload") =3D=3D 0 || > + strcmp(action, "status") =3D=3D 0) { > + > + while (*services !=3D 0) { > + if ((service_filter !=3D NULL && strcmp(service_filter, *servi= ces) =3D=3D 0) ||=20 > + service_filter =3D=3D NULL) { > + if (initscript_action(*services, action) !=3D 0)=20 > + r =3D 1; > + ++actioned; Wouldn=E2=80=99t it be a better ideal to pass the service_filter argument to = the action function and only filter once in that function? Otherwise you are copying the same code over and over again. Or have a helper= function that will be called from initscript_action, toggle_service and so o= n? > + } > + ++services; > + } > + > + } else if (strcmp(action, "enable") =3D=3D 0 || > + strcmp(action, "disable") =3D=3D 0) { > + > + while (*services !=3D 0) { > + if ((service_filter !=3D NULL && strcmp(service_filter, *servi= ces) =3D=3D 0) ||=20 > + service_filter =3D=3D NULL) { > + if (toggle_service(*services, action) =3D=3D 0) { > + fprintf(stdout, "%sd service %s\n", action, *services); > + } > + else { > + r =3D 1; > + fprintf(stderr, "\n%s\n\n", errormsg); > + } > + =20 > + ++actioned; > + } > + ++services; > + } > + > + } else if (strcmp(action, "boot-status") =3D=3D 0) { > + while(*services !=3D 0) { > + if ((service_filter !=3D NULL && strcmp(service_filter, *servi= ces) =3D=3D 0) ||=20 > + service_filter =3D=3D NULL) { > + print_boot_status(*services); > + ++actioned; > + } > + ++services; > + } > + =20 > + } else if (strcmp(action, "list-services") =3D=3D 0) { > + fprintf(stdout, "\nServices for addon %s:\n", addon); > + while (*services !=3D 0) { > + fprintf(stdout, " %s\n", *services); > + ++actioned; > + ++services; > + } > + fprintf(stdout, "\n"); > + > + } else { > + fprintf(stderr, "\nBad argument given.\n\n%s\n\n", usage); > + r =3D 1; > + } > + > + if (r =3D=3D 0 && service_filter !=3D NULL && actioned =3D=3D 0) { > + fprintf(stderr, "\nNo service %s found for addon %s. Use 'list-ser= vices' to get a list of available services\n\n%s\n\n", service_filter, addon,= usage); > + r =3D 1; > + } > + > + // Cleanup > + for(int i =3D 0; i < servicescnt; i++)=20 > + free(services_ptr[i]); > + free(services_ptr); > + > + return r; > } > --=20 > 2.37.3 So all in all, this is the way to go. Since we have all this metadata now, we= should use it, and this is a very good place to start. However, there are some problems in this implementation and since this code i= s gaining root privileges, we need to make sure that it is 100% safe. Safe ag= ainst any garbage inputs that might have ended up here, safe against some mak= ing some easy mistakes when the script is being changed again next time. So b= asically, do our best here. Sorry to be so strict, but the setuid binaries ar= e a massive pain point in the current web UI and therefore we now have the gr= eat pleasure to fix old design issues. -Michael >=20 >=20 > --=20 > Dit bericht is gescanned op virussen en andere gevaarlijke > inhoud door MailScanner en lijkt schoon te zijn. >=20 --===============0382553591277405763==--