CodeSOD: Authorized Users

"Hey, since Jack quit, you could add Jane to the investigation users?"

William M supported one piece of a large, complicated intranet application used by auditors as part of investigations into "events" within the organization. The overall audience is small, and the purpose of the application is mostly to document the process of the investigation. Since it's so niche, it doesn't even have a screen for managing user accounts- the users raise a ticket and rope in developers to manage it.

The developer in charge of the secure portion of the application- Rick- was busy, so William caught the ticket. He wasn't sure what to do, so he shot an IM over to Rick.

"Oh, it's easy," Rick replied. "Just update Jack's record with Jane's user info. That's the easiest way, since Jack quit."

That didn't seem like a smart process, but William was in a rush and this was Rick's baby, so he just followed instructions and closed the ticket. Jane re-opened the ticket- she couldn't access the audit compliance tracker. Jack had never used that page, but Jane planned to.

Rick was still busy, so William decided to take a look at the authorization process. He found this code in the main-menu screen of the application.

if (Convert.ToInt32(Session["UserID"]) == 201 || Convert.ToInt32(Session["UserID"]) == 226 || Convert.ToInt32(Session["UserID"]) == 214 || Convert.ToInt32(Session["UserID"]) == 258 || Convert.ToInt32(Session["UserID"]) == 260 || Convert.ToInt32(Session["UserID"]) == 22 || Convert.ToInt32(Session["UserID"]) == 303 || Convert.ToInt32(Session["UserID"]) == 444 || Convert.ToInt32(Session["UserID"]) == 32 || Convert.ToInt32(Session["UserID"]) == 388 || Convert.ToInt32(Session["UserID"]) == 413 || Convert.ToInt32(Session["UserID"]) == 392){ btnSecureArea.Visible = true;}

"No" no way."

William checked one of the other secure pages.

int userID;if (!int.TryParse(Session["UserID"].ToString(), out userID)){ Response.Redirect("NavigationPage.aspx");}if ( !(userID == 201 //Jerry || userID == 226 //Lisa || userID == 214 //Alban || userID == 258 //David || userID == 22 //Jack || userID == 444 //Tyrell || userID == 303 //Brenda || userID == 413 //Phung || userID == 388 //Ignacio || userID == 399)//Norma ){ Response.Redirect("NavigationPage.aspx");}

This wasn't looking so great. William pulled up the code for the audit compliance tracker that was causing Jane trouble.

if (Convert.ToInt32(Session["UserID"]) == 201 ||//Jerry Convert.ToInt32(Session["UserID"]) == 226 ||//Lisa Convert.ToInt32(Session["UserID"]) == 214 ||//Alban Convert.ToInt32(Session["UserID"]) == 258 ||//David Convert.ToInt32(Session["UserID"]) == 303 ||//Brenda Convert.ToInt32(Session["UserID"]) == 32 ||//Clementina Convert.ToInt32(Session["UserID"]) == 199 ||//Shyla Convert.ToInt32(Session["UserID"]) == 232 ||//Harmony Convert.ToInt32(Session["UserID"]) == 413 ||//Phung Convert.ToInt32(Session["UserID"]) == 388 ||//Ignacio Convert.ToInt32(Session["UserID"]) == 399) //Norma{}else{ Response.Redirect("NavigationPage.aspx");}

Well, it wasn't hard to see why Jack-now-Jane's user id got kicked out of the page. With all the copypasta, things had gotten out of sync. Even better, this code was inside of the Page_Load handler on a WebForm, and since it didn't return on failure, the rest of the Page_Load event executed, even though the user didn't have permission to access the page.

William didn't have the time or permission to actually move this data into the database, but he did spend the time to refactor the copy/pasted code into a single method that used a list of IDs. By making sure the authorizer quits on failure, he also fixed half-a-dozen low-priority "unexplainable" bugs caused by side effects from the Page_Load executing.