From d68566755978c647803c32f9afe126a16a121eac Mon Sep 17 00:00:00 2001 From: Kohei Tamura Date: Mon, 11 Sep 2017 17:09:59 +0900 Subject: [PATCH] Improve error handling --- .../controller/DefaultLoginController.java | 70 +++++++++++-------- .../OpenRedirectController.java | 18 ++--- .../SQLInjectionController.java | 8 +-- .../VerboseErrorMessageController.java | 24 ++++--- 4 files changed, 66 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/t246osslab/easybuggy4sb/controller/DefaultLoginController.java b/src/main/java/org/t246osslab/easybuggy4sb/controller/DefaultLoginController.java index bad362c..b889073 100644 --- a/src/main/java/org/t246osslab/easybuggy4sb/controller/DefaultLoginController.java +++ b/src/main/java/org/t246osslab/easybuggy4sb/controller/DefaultLoginController.java @@ -31,25 +31,25 @@ @Controller public class DefaultLoginController { - @Value("${account.lock.time}") - long accountLockTime; + @Value("${account.lock.time}") + long accountLockTime; - @Value("${account.lock.count}") - long accountLockCount; + @Value("${account.lock.count}") + long accountLockCount; @Autowired protected MessageSource msg; - + @Autowired - protected LdapTemplate ldapTemplate; - + protected LdapTemplate ldapTemplate; + /* User's login history using in-memory account locking */ protected ConcurrentHashMap userLoginHistory = new ConcurrentHashMap<>(); - + private static final Logger log = LoggerFactory.getLogger(DefaultLoginController.class); - - @RequestMapping(value = "/login", method = RequestMethod.GET) - public ModelAndView doGet(ModelAndView mav, HttpServletRequest req, HttpServletResponse res, Locale locale) { + + @RequestMapping(value = "/login", method = RequestMethod.GET) + public ModelAndView doGet(ModelAndView mav, HttpServletRequest req, HttpServletResponse res, Locale locale) { mav.setViewName("login"); mav.addObject("title", msg.getMessage("title.login.page", null, locale)); @@ -63,14 +63,15 @@ public ModelAndView doGet(ModelAndView mav, HttpServletRequest req, HttpServletR HttpSession session = req.getSession(true); if (session.getAttribute("authNMsg") != null && !"authenticated".equals(session.getAttribute("authNMsg"))) { - mav.addObject("errmsg", msg.getMessage((String)session.getAttribute("authNMsg"), null, locale)); + mav.addObject("errmsg", msg.getMessage((String) session.getAttribute("authNMsg"), null, locale)); session.setAttribute("authNMsg", null); } return mav; } - @RequestMapping(value = "/login", method = RequestMethod.POST) - public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServletResponse res, Locale locale) throws IOException { + @RequestMapping(value = "/login", method = RequestMethod.POST) + public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServletResponse res, Locale locale) + throws IOException { String userid = StringUtils.trim(req.getParameter("userid")); String password = StringUtils.trim(req.getParameter("password")); @@ -95,7 +96,7 @@ public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServlet session.setAttribute("authNMsg", "authenticated"); session.setAttribute("userid", userid); - + String target = (String) session.getAttribute("target"); if (target == null) { res.sendRedirect("/admins/main"); @@ -105,18 +106,19 @@ public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServlet } } else { /* account lock count +1 */ - User admin = userLoginHistory.get(userid); - if (admin == null) { - User newAdmin = new User(); - newAdmin.setUserId(userid); - admin = userLoginHistory.putIfAbsent(userid, newAdmin); + if (userid != null) { + User admin = userLoginHistory.get(userid); if (admin == null) { - admin = newAdmin; + User newAdmin = new User(); + newAdmin.setUserId(userid); + admin = userLoginHistory.putIfAbsent(userid, newAdmin); + if (admin == null) { + admin = newAdmin; + } } + admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); + admin.setLastLoginFailedTime(new Date()); } - admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); - admin.setLastLoginFailedTime(new Date()); - session.setAttribute("authNMsg", "msg.authentication.fail"); return doGet(mav, req, res, locale); } @@ -124,22 +126,28 @@ public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServlet } protected boolean isAccountLocked(String userid) { + if (userid == null) { + return false; + } User admin = userLoginHistory.get(userid); return admin != null && admin.getLoginFailedCount() == accountLockCount - && (new Date().getTime() - admin.getLastLoginFailedTime().getTime() < accountLockTime); + && (new Date().getTime() - admin.getLastLoginFailedTime().getTime() < accountLockTime); } protected boolean authUser(String userId, String password) { + if (userId == null || password == null) { + return false; + } try { /* Perform a simple LDAP 'bind' authentication */ - LdapQuery query = LdapQueryBuilder.query().where("uid").is(userId); - ldapTemplate.authenticate(query, password); + LdapQuery query = LdapQueryBuilder.query().where("uid").is(userId); + ldapTemplate.authenticate(query, password); } catch (EmptyResultDataAccessException | AuthenticationException e) { return false; - } catch (Exception e) { - log.error("Exception occurs: ", e); + } catch (Exception e) { + log.error("Exception occurs: ", e); return false; - } + } return true; - } + } } diff --git a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/OpenRedirectController.java b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/OpenRedirectController.java index d8c44c3..e64b683 100644 --- a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/OpenRedirectController.java +++ b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/OpenRedirectController.java @@ -85,17 +85,19 @@ public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServlet } } else { /* account lock count +1 */ - User admin = userLoginHistory.get(userid); - if (admin == null) { - User newAdmin = new User(); - newAdmin.setUserId(userid); - admin = userLoginHistory.putIfAbsent(userid, newAdmin); + if (userid != null) { + User admin = userLoginHistory.get(userid); if (admin == null) { - admin = newAdmin; + User newAdmin = new User(); + newAdmin.setUserId(userid); + admin = userLoginHistory.putIfAbsent(userid, newAdmin); + if (admin == null) { + admin = newAdmin; + } } + admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); + admin.setLastLoginFailedTime(new Date()); } - admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); - admin.setLastLoginFailedTime(new Date()); session.setAttribute("authNMsg", "msg.authentication.fail"); res.sendRedirect("/openredirect/login" + loginQueryString); diff --git a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/SQLInjectionController.java b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/SQLInjectionController.java index f23391b..72bafce 100644 --- a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/SQLInjectionController.java +++ b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/SQLInjectionController.java @@ -45,10 +45,10 @@ public ModelAndView process(@RequestParam(value = "name", required = false) Stri } else { mav.addObject("userList", users); } - } catch (DataAccessException se) { - log.error("SQLException occurs: ", se); - mav.addObject("errmsg", msg.getMessage("msg.db.access.error.occur", null, locale)); - } + } catch (DataAccessException se) { + log.error("DataAccessException occurs: ", se); + mav.addObject("errmsg", msg.getMessage("msg.db.access.error.occur", null, locale)); + } } else { mav.addObject("errmsg", msg.getMessage("msg.warn.enter.name.and.passwd", null, locale)); } diff --git a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/VerboseErrorMessageController.java b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/VerboseErrorMessageController.java index f0cc956..12a62fd 100644 --- a/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/VerboseErrorMessageController.java +++ b/src/main/java/org/t246osslab/easybuggy4sb/vulnerabilities/VerboseErrorMessageController.java @@ -77,17 +77,19 @@ public ModelAndView doPost(ModelAndView mav, HttpServletRequest req, HttpServlet } } else { /* account lock count +1 */ - User admin = userLoginHistory.get(userid); - if (admin == null) { - User newAdmin = new User(); - newAdmin.setUserId(userid); - admin = userLoginHistory.putIfAbsent(userid, newAdmin); - if (admin == null) { - admin = newAdmin; - } - } - admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); - admin.setLastLoginFailedTime(new Date()); + if (userid != null) { + User admin = userLoginHistory.get(userid); + if (admin == null) { + User newAdmin = new User(); + newAdmin.setUserId(userid); + admin = userLoginHistory.putIfAbsent(userid, newAdmin); + if (admin == null) { + admin = newAdmin; + } + } + admin.setLoginFailedCount(admin.getLoginFailedCount() + 1); + admin.setLastLoginFailedTime(new Date()); + } session.setAttribute("authNMsg", "msg.password.not.match"); return doGet(mav, req, res, locale);