 All right, thanks everybody for coming. I will start two minutes early because the room is full already anyway. So my name is Klaus Purer. I've been a Drupal contributor for a long time. And my specialty is code review. So I have new contributors. When they publish the module, they have to go through this process called the security advisory coverage process. And my screen just went out. Damn, new screensaver. I hope it's back soon. You can find me on drupal.org under Klausi. I was also part of the Drupal security team for a couple of years. I work for Tropicode.com. We are a job work provider that is doing their technology based on Drupal. And yeah, during this process, when I do the code reviews, I thought it would be a good idea to somehow give some examples what kind of stuff that I find so that you can also apply that when you review the code of your colleagues or code on drupal.org, whatever. What are the goals of the code review? What are we trying to accomplish? So we want to find problems before they hit production. So somebody is writing the code, and then it would be a good idea if somebody is actually reviewing the code and making sure that nothing bad is in there. And that way, we can prevent security vulnerabilities before they are exploited. So we are in the security process that is called the prevention phase. So before anything actually happens, we make sure that we prevent as much as possible. Code review is also a form of knowledge transfer. So the developer that is reviewing the code might know some things that the author of the code didn't know, and they can point it out, and that way that the code can be improved. And actually, that's also a good opportunity for new people in your company or new contributors on drupal to point in the right direction how APIs should be used. And in the end, what do we want to do as companies? We want to save costs by preventing problems. We want to prevent bugs so that it's not embarrassing for us when clients detect those bugs. We want to catch them beforehand. But we also want to save costs and prevent the security breach, for example, or any vulnerability that would expose our data or compromise our data. Usually, code review is done in pull requests. So that happens a lot on GitHub. But we also have the patch system on drupal.org. And within your company, I don't know what to use. Many companies also use pull requests. And the goal is one, developer writes the code and the peer reviews the code and makes it better. All changes to drupal core, for example, are peer reviewed and code reviewed. So before a change goes into drupal, it has to be looked at by another contributor. And then they approve it. And then it gets committed by core committers. So we're also doing that in drupal already. So yeah, I structured my talk by giving just a couple of examples. Let's look at the typical drupal 8 module layout. In this example, I used the name PANTS. So we have some kind of PANTS module. Not sure what it does. The typical files that you are seeing in this module is install files, module files, and then a couple of YAML files in drupal 8 that contain your configuration. What I look at first is the module file and the routing file, because when I first look at the module, I want to see the attack entry points. What can somebody do with the module? Where's the request coming in? And where's the request handled? And what problems might we see there? Then I go through the controllers and classes and services that we have in the drupal 8 module. And in the end, I want to know where the information goes out. So I would be looking at the tweak templates, preprocess functions, anything that creates output in drupal render arrays, what they are doing. That's my typical process. And in the end, I want to read through the whole code of the module. I think that's also important. It's not good if you review parts of a module. In the end, you should just take a look at every file, what they are doing in there. Then you also better understand how they are interacting. So let's start with routing. Routes are defined usually in the routing YAML files. You can also create dynamic routes in some dynamic route plugin. I don't know if it's a plugin. I think it's an event subscriber. But usually, people do that in the routing YAML file. So this is where I look first. Incoming requests, they are mapped in these routing files to controllers. And here, for example, we have a PANTS controller in the source folder. So that's just to look at the routing file, what is defined there, and then the PANTS controller, what it's actually doing. This is not the only attack entry points that you can have in a Drupal module. So Drupal is also known to do a lot with events, plugins, and hooks. So those would be the next extension points that you would look at in a module after you are done with routing. All right, so let's take a look at this PANTS routing YAML file. Let's read through it. The route has a name at the top, PANTS.admin. It exposes something at a path called slash admin slash PANTS. And there's a form attached. It's the PANTS admin form. And in order to access that admin form, you have to have the permission access content. And we define it as an admin route. So not sure I like that. Any idea what might be wrong here? Access content? Yeah, access content is really a weird permission that shouldn't be here. So this would be an access bypass in a route. I don't know what this admin form does, but just guessing from name, I know already admin stuff. We don't use the access content permission. We need to probably use some other permission there, like administer PANTS, for example, if it's about the PANTS module. Some people also use like administer site configuration, which is the very generic backend configuration permission in Drupal that is a bit overused. So better module defines their own admin permission and use that, but don't use access content for admin routes, for example. And I tried to map this a bit to the OWASP top 10 security vulnerabilities. OWASP is a project that collects risks, vulnerabilities, and how they can exploit it in this comprehensive documentation framework. It's the open web application security project. And actually this would fall under the top five called broken access control. So some access control is broken and a attacker that has access content could actually change something on this admin form. But I even see even worse things. So in the requirements, if it's by specify underscore access true, it means everybody can access this admin form. Why do people do this? Why do I see this in code reviews so often? So people, they prototype with their module and they don't have a permission yet. And just for development, I will put in this access flag true and then later I will fix it. Of course that later never happens and then this stays in the module and then the module might be deployed and then this is really dangerous because any attacker, actually any user can access this path and change values in the form. Yeah, so that's a second access bypass. Let's look a little bit more complicated example. Now we want to delete pants. So pants are some entity, some object in our group system, we don't care actually for this example. And we have a delete path where I can specify an identifier of a path. For example, in number one, two, three, four, five whatever the identifier of the pants is. Then it goes through to the delete controller. We specify it in the controller here. Some pants module is loaded and in the controller as you see at the bottom, the pants are deleted and then we show a success message in a Drupal render array. So this looks good, right? What could possibly go wrong here? I see you're raising a hand here, any idea? Regular expressions? No, regular expressions should be a problem here. So if you just pass an ID, that's not the point I want to make here. Any other guesses what might be wrong with this code? No confirmation step. Exactly, and what does no confirmation step mean? This is a cross-site request forgery vulnerability in a route. So we never confirm in the user's intent here. You have to imagine if I just type in slash pants slash delete slash one in the browser and you do this as an admin, it will just go ahead and delete this pants without user confirmation. What do we do in Drupal to prevent that? Can we use confirmation forms or tokens in the URL or the request? So how could an attacker exploit this? They put together an image tag and instead of using an actual image in the source, they use this path to your Drupal site where they point to some specific pants they want to be deleted on your behalf. And that's actually also the root of the problem. The browser is doing something on your behalf. So you visit the attacker page and there is this image. And the image source fetches something from your page from your Drupal site. And what the browser does, it also sends the session cookie along. So you're logged in as admin on your Drupal site. The browser fetches this image, sends the cookie along and then something on a completely different site is just executed with a get request, right? Because there is no form. There's no step in between. The pants just get deleted and then it's done. So the pants delete controller executes your callback and suddenly pants five are gone. You didn't intend to do this, right? And that's why we need confirmation form in Drupal when you delete the node you get a confirmation form. When you do data changing actions in your Drupal code, then you need to confirm the user's intent. And another way to do that, sometimes you don't want a confirmation form because it's bad for usability. If you're deleting something unimportant which we don't care about, then you might not want a confirmation form and you can use something called CSRF tokens. And Drupal 8 has some nice support for that. So you put this CSRF token property into your routing yaml file. So this is the same routing yaml file that we have seen before except that we have added this CSRF token entry and we say true. And then we have an output link to delete those pants. Such a cryptographically secure token is appended and this token has a different value per admin user. It has a different value per session so the attacker cannot guess it. The attacker cannot put an image on their side and come up, I will use this in this token as we surely work now. It is generated specifically for an administrator so an attacker cannot recover it. So if this token is missing, what happens then? Drupal will return an access denied for this page. So this attack that we've seen before with the image wouldn't work anymore because of course at the end of the source here the token is missing. And where do we use these tokens? If you don't have a confirmation form, so for example the shortcut module in Drupal Core, you know it, you can create little shortcuts somewhere in the admin interface and then you can quickly jump to an admin page. And if I delete such a shortcut having a confirmation form would be tedious. Yes, I really want to delete this shortcut. So although it is a data changing operation, right, I'm deleting something and delete the shortcut. It's not that important. So even if the user clicks it by accident then this is also executed and the shortcut is deleted. It's not a big problem because they can just quickly create a shortcut again. You would never use such a CSI token for a node for example. Let's say you have huge block articles with very important content to you then you wouldn't use this token because then a node would be deleted immediately so you would rather use confirmation forms. So by default always use confirmation forms for delete operations for examples or for your forms when you edit the node there's of course also forms because forms by default in Drupal are CSRF protected. So that's already built in and if you do need something fancy for better usability there is this CSRF token helper which only exists in Drupal 8 so in Drupal 7 you have to kind of do the CSRF token validation on your own so Drupal 7 is not nice in that regard. Yeah, so a couple of Drupal 7 examples so what we saw in the Yammer files when you review a Drupal 7 module this is basically one-to-one the same concept except that it's in hook menu which is defined in the module file. There the routing is defined there you have the title page callbacks and whatever and as we saw before the access content permission is wrong here again because we are rendering an admin form and it should have an adequate permission. The same for the access callback true we have this in Drupal 8 we have this in Drupal 7 so when I look into a model I always scan the routing parts if the developer forgot to set the access callbacks correctly and then I can point out maybe a security vulnerability. So everything that we saw in Drupal 8 also applies here and when I review this and maybe a developer does this on purpose there might be a payment gateway which has to have an open access page callback because paper is calling back and paper always needs to have access so we define it like that so it would be actually correct and then I tell developers please leave a code command above it. So above the command they should write a comment there is no access protection here because paper always needs to reach this page and then the reviewer knows aha they did it on purpose and it's not an accident. We also have the same patterns for course write request directory in Drupal 7 routing is defined in hook menu again and then we have some page callback which deletes something pants in this example and we also should use confirmation forms here same as in Drupal 8 we don't have automatic CSF tokens so if I want to validate some token and it would have to do this myself in this page callback a couple of contrib modules are doing that I think you can even find it in Drupal 7 core so you would copy this token generating and token validating code from there. All right so some attacker entry points with routing summarized in Drupal 8 you have routing gamma files we have first look at and then in the source folder the controllers and the forms they are important to me because those are things that an attacker potentially can directly reach and in Drupal 7 the equivalent is hook menu and page callback in modular ink files so module files are always important in Drupal 7 and in 8 so you should always review those and as I said to check for access bypass with permissions how the access handling is done and also for a cross-site request for a tree so whenever you see an action in a routing file like delete something or change something I immediately go checking is this a confirmation form is it just a controller and then I already think there might be some CSF vulnerability here. Now for something different a bit so we see this method of a class it gets log items since some time stamp and what the developer did here they wrote a query with the database API all is looking good they are writing a WHERE query here and then they execute this WHERE query and return the results of stuff that they get from the database. SQL injection SQL injection I heard, yeah, correct so whenever you see string concatenation or whenever you see variables embedded in a string which is related to database query you shouldn't do that because what's the problem? So we are passing off information to another system and this other system is the database and to the database we are sending instructions and we are sending data but if we put the instructions and the data together the database cannot distinguish anymore they just get the instructions and execute it the database doesn't know what is coming from an attacker and what has been done by the developer so this is one of the top risks in the OBASP top 10 it's actually number one because it's so, it's a high risk it can happen often people often get it wrong called injection it doesn't only apply to SQL injection there are also other injection vulnerabilities and yeah so whenever you see string concatenations or we are variables embedded in a string that's a code smell that something is wrong here actually this might be hard to exploit in a WHERE clause in this specific case so when the timestamp is attacker supplied so the attacker puts in some query tries to insert something that might not actually work because the WHERE query part in Drupal might sanitize this but you should still never do it and the correct way to actually do this is the condition API here where we clearly have the separation the name of the property want to query and then there is a specific parameter for the user supplied value the timestamp in this case and then this is safe because the database API escapes it correctly and then the instructions sent to the database the actual SQL query escapes that data correctly all right what do we have here so we want to extract content from a PDF and PHP is not actually good at that so we want to use Apache Tica which is a Java library we wrote this little function where we can specify where this library lives on our server it lives in opttica app.jar and then we build a command and then we execute the command and this shellexec function that you see here it will just execute the command and then return the standard output well, I guess that works any problems anybody sees here? right, so the variables that we are passing to the command they are not escaped so you can have a remote code execution in your shell it's called shell injection so if this URL that is coming in this is coming from user data then the attacker can execute for example a second command so let's imagine something from URL something like this so they specify some URL but then they use the semicolon to execute a second command on bash for example to give me the contents of slashetc slashpassvd where we have some user information there that should never be leaked from any server so then this is executed together with Tica Tica runs on this URL whatever the URL is Tica generates some output then a second command is executed which also generates some output so the output gets together in this parsed variable and the parsed variable is returned and not only did I extract code from a PDF document suddenly I also get the contents of this file on the server so if an attacker can supply this URL this is really dangerous you have shell injection they can execute arbitrary things on your server how do we prevent that? bitb has a bit in function called escape shell arc for arguments so whatever goes into that command needs to be escaped with escape shell arc and then we are safe so I'm not only doing this for the URL here but I'm also doing it for the variable because it can't hurt so the Tica lip path is also escaped it probably doesn't have to be escaped because it's provided by the developer but what you want to do when you're writing Drupal code you always want to have a defense in depth approach so even if an attacker is able to inject something into the variable table and we get it out here even then we would escape it so they wouldn't have a chance here so that's always good to have a layered approach to your security that you're not protecting yourself in one place but in multiple places if it doesn't interfere with the operation of your Drupal site of course but in this case it's totally fine to escape this static string and it will still work and even better maybe avoid using shell exec all together maybe you can use Apache Tica as a service and an HTTP request and that is also much safer and you can avoid that so in Jackson's summarized we look into page callbacks and everything that executes query or shell scripts typically those are controllers or forms in Drupal 8 we also look in API functions that are used in services in Drupal 8 somebody would probably provide a service and do the shell exec there so that's also a good place to look and whenever you see concatenating strings instead of using placeholders that's a very, very good indicator that either the command is vulnerable or the SQL query is vulnerable so look for that what do we have here this is an access hook for hook taxonomy term access and it forwards the in the first if clause it forwards the code to another static function whatever so this is actually real code as I saw in a Drupal module and then it checks if we get an access result allowed there I'm also returning an access result allowed so I think it looks correct look what could go wrong here so we have this if it looks a bit long yeah I think it's hard to recognize do we have any problem with the first if? I think we do because we have an access result if bypass so from the function from a method below you get an access result and then you check if access result but that will always be true the problem is if you just have if access result that is an object in this case so because the things below they return objects so we have an object here and of course an object always evaluates to true so access will always be granted in this case so always review your access hooks for that access results are used correctly and for access checks always use access result result is allowed another example this was actually from the GraphQL module which I maintain so I find security issues in my own modules so this happens so we have some result function and it loads some entity from somewhere and then we have the same access result again by calling access on this entity and the true parameter at the end means return it as an object so I want to use it as an object because I need to add it to my casual dependency tree here that's the reason why I get this object and then I call if access result is forbidden do nothing and otherwise return the entity this is also a problem because it's the access result is forbidden track that's what I call it in Drupal 8 the access result can have three states so we have allowed forbidden but you have also neutral as we have seen before already and you have to call access result is allowed because it's forbidden micro return false or neutral results so that's a bit I think a weakness of the Drupal access result API in Drupal 8 and this was this is forbidden called was overlooked by three very senior developers in the GraphQL module and which has recently found out about it and what's the correct fix which has called access result is allowed because that takes into account the neutral result which should also forbid access correctly so it's important to familiarize yourself with the access API in Drupal 8 when you write this access code and make sure that the logic is correct it even has this quote in the documentation important note you have to call is allowed when you want to know whether someone has access yep the problem is the other and the other methods are also available and PHP is gone or which is to your code whatever editor you use just auto completes them for you so you might want to use them but don't always use is allowed so access control summarized we have to check the logic that we do in the access controllers and the hook entity access which exists in Drupal 8 and in Drupal 7 and we need to check if the if conditions are correct and whenever access result is lose use this object we should check for is allowed and know other methods called to determine access to get a boolean out the only right way to get a boolean out of the access result is to call is allowed also the cache context information can also be security critical so make sure that you catch the correct things you don't want to cache an unpublished note and then suddenly return it for users that shouldn't have access to unpublished notes yeah so also check that now some example for render race and cross-site scripting so cross-site scripting happens on output so I'm switching out to the output side of reviewing I typically would look into page controllers they are generating outputs and tweak templates and what an attacker is able to supply in this example is a script tag so it would execute some JavaScript and now I'm putting this JavaScript variable into a render array what do you think is this okay would an alert pop-up pop-up in Drupal 8 or do you think it is safe so the answer is a bit complicated here in Drupal 8 this is safe because the render system runs XSS filter admin on it so this is a very good improvement in Drupal 8 where all the markup that we are generating all the things that we are outputting in tweak is how to escape the default so we try to escape everything against this cross-site scripting attacks by default but unfortunately in Drupal 7 it is not safe because the render system just prints whatever there is so whenever you have user provided an input and put it in markup tags or prefix tags whatever you have to be very careful this is the top seven vulnerability listed by the OWASP so I encourage you to check that out let's look at a tweak template in this example so tweak is usually very good it does auto-escaping on all the variables but there is something in there which is actually an XSS vulnerability it's already highlighted by the syntax highlighter here and this is this little raw keyword so whenever you read your templates check if a developer uses this raw keyword because it means auto-escaping is disabled and the variable is rendered as is so this would be a template cross-site scripting vulnerability so whenever you look at templates check if they use the raw filter how can you actually test for cross-site scripting? so I'm always using this alert XSS snippet I put this in all inputs in get parameters whatever and then I see if it's coming out of Drupal again and if it's coming out then I get this annoying JavaScript pop-up and I know something is not as tape correctly this is very helpful for Drupal 7 specifically but it's also useful in Drupal 8 but tweak isn't perfect, right? if I'm doing something very stupid like this so putting the node label, which is the node title directly into the source attribute without using quotes that's not good because it bypasses auto-escaping actually auto-escaping is happening but it's not really helping here because the attacker can put in something like this so you have this A this A is then used as the source of the image of course it doesn't load so an error is thrown so the browser executes the unerror attribute which would then open a pop-up and this is just an example but an attacker could provide some other JavaScript there it's a bit tricky with the quotes so the attacker would have to be very careful but it's possible to have this so make sure that you have the correct quotes in your templates don't forget them because then the auto-escaping doesn't help you the auto-escaping mostly helps you between HTML tags but if you're working with attributes in HTML you have to be a lot more careful what you're putting in there and there is the over-aspect cross-site scripting sheet I encourage you to check that out as well it has some examples like this one what people can put in attributes so they can also trigger cross-site scripting oh yeah, that would then be the resolved version we have this as the node title and then this is injected into the template and this would be the image tag and then the pop-up would trigger so cross-site scripting summarized and the auto-escaping is really good in Drupal 8 be careful with the HTML attribute as I said in Drupal 7 it's unfortunately a bit of a nightmare because you have to check so many things there could be cross-site scripting in the PHP templates and the team functions pre-process in the callbacks where you vary your building and this but there's a good documentation page called Handle Tags in a Secure Fashion I linked it here from the slides and I will post the slides later another vulnerability we had this year was the un-serialize vulnerability that is executed on user-provided data so un-serialize is used to take an array or an object and put it into a string so that you can send it over the network and what is happening here even with get requests if you have the REST module and some REST resource enabled and this request is incoming then Drupal tries to deserialize that an attacker would put an evil serialized payload in this options value and then this would be deserialized and there was a vulnerability in core which just deserialized everything and that's how it happened and then you have remote code execution why do we have a remote code execution? you can take a look at some evil un-serialized payload that would be supplied so an attacker is specifying O which means objects and then some class in Drupal in this case it was something from the Gazel project which is a dependency of Drupal core and this special class can hold some properties which execute arbitrary PHP code and what an attacker does they supply the pass-through function which is a PHP function to just execute something on the shell and then an evil command in this case it's very primitive it's just echo hacked something happened but of course an attacker would put something more severe in there and then what Drupal does or what it has done it has been fixed in the meantime it deserializes this then we have this Gazel object and then the Gazel object runs out of scope then the PHP garbage collector runs and then this object is destroyed and when the object is destroyed then this page core not this page core but this shell command is executed and that would be the remote code execution I linked some exploit code also in the notes of the slides how somebody abused that so what do we learn from that and we need to use unserialized correctly this is not showing up that often in the in contributed modules that you see but whenever data is coming in we need to make sure that we call unserialized correctly we can just disallow our classes because what is Drupal using serialized stuff with usually it's arrays it's some option array that we have been seeing some properties on the link and then we don't need classes we can just disable it and then unserialized is safe and actually unserializing stuff has been a vulnerability in many programming languages and frameworks and that's why the OS was classified also in the top 10 list ranking eight in security serialization we also have a vulnerability with XML entities so if you're passing XML you also have to be careful because you can have special XML documents like this and what we are doing here is it's called entity expansion and this is part of the XML standard which is not good for internet applications so what it's trying what it's doing is it's just commands that are then filling XML content so in this case it would read data from ADC passivity and then fill it into this full text and if the attacker gets back this XML somehow they are sending the XML and you are outputting it somewhere or processing it then it would contain the content of this file on the server which an attacker shouldn't have access to it's also on the OS top 10 list actually is number four so what can you do in PHP about that you would disable the entity loader in the libxml library so PHP is using a C library to pass XML and there you can just disable this and then this expansion just wouldn't work it would just not expand anything it would just leave the content as is and that's how you can deal with that so it's actually a simple fix but whenever you work with XML in PHP you shouldn't forget it all right so those were just some examples and what so let's summarize what do I do on a full code review rundown I checked the ng points in the Drupal module as we had routing files, module files is the cross-site request for tree what is happening in the module files then I check how is the module dealing with input so on input we always want to validate what is coming in that is usually done in controllers are they having some SQL injection vulnerabilities with the input or deserialized vulnerabilities that we saw and in the end I check where the data is coming out of the module again it rendered into templates or something else checking for cross-site scripting there important is to check also the access control this is usually done in Drupal, plugins, hooks whatever and then I always make sure to read through the rest to understand completely what the module is doing and what you should always do when reviewing you should get into the attacker mindset how could the code be abused what could go wrong with this code and security professionals they always recognize security as a process so code review is only one small part of everything that you should do the easier thing that you need to do is just keeping your software components up to date because they're known vulnerabilities and we all use Drupal, we all use existing contrib modules just updating them helps you to keep that secure we need to make sure that everybody is aware and trained for security so we want to deploy a defense in depth approach that we secure a server or a Drupal code or custom modules and also the people people shouldn't for social engineering attacks where somebody calls and says please give me your password, yeah don't do that, people should be aware of that and then code review is part of the quality assuring thing with testing where we try to prevent security vulnerabilities but we should also have a plan in case anything happens the good frameworks what you can do on incident response when the Drupal site is hacked what you need to do having backups in place and then you can also have penetration tests where you get some external company to validate your code some resources I have there is the OVAS code review guide it's fairly long I think it's a really long document it's a lot of words I don't particularly like it it's just too much text for me but I think it's a good framework to understand like even from the outline what you can do in code review there was top 10 security risk I already mentioned there was also a session at this Drupal console what we're recording from that one in Drupal 8 we have documentation about writing secure code so it's also important for every developer to understand that and what I like to work with are there's Drupal security coverage applications where we review modules of new contributors that want to get security coverage for their modules and I'm reviewing what is there if you want to know, learn more about that there's also a link and with that, that's it any questions? so there's a microphone we should try to get it on the recording so if anybody, or I can also repeat the question so yeah, Danny back there's something I've missed from your talk is code testing because the access test should be called by many tests yes, absolutely so code review is only one part with code review you can just find stuff that has not been covered by the tests or where the tests might actually be wrong so yes, automated tests should also check for security that's very important so that your tests for scenarios which could be abused by attackers so what people usually often test is the happy path they test something, they save a note and then the note shows up but they don't test the thing where they put in some dangerous stuff in the note title and what is then showing up and sometimes people don't think of all the possibilities what can go wrong with the module or the code and then they don't catch it with automated tests and that is actually so I think these are both sides of quality assurance that you need to do peer review is one side to get the perspective of a human and automated test is something to ensure what the developer wrote is actually correct yeah so the code review is validating what the developer did from an external standpoint and automated tests is the developer making sure herself that what she did is actually correct yeah yes yeah quite a few of these are probably repeating all the time everywhere yeah can these be or are they included checks against these in any of the coding linters or anything like that yeah so the question was if we can automatically detect this common vulnerabilities that I just shown in those examples the question is we don't really have something like that so I'm also the maintainer of the Coda library which is checking for coding standards and we have something in there called Drupal practice so this is a scanner which tries to give you hints on what you might be doing wrong in your code but it's just it's a static analyzer so it doesn't have the full information what is actually going on and especially that's a problem in Drupal 8 where you have complex object dependencies and you might not know what the method call actually means and if this is a bad method call and people should do something else but since we wrote libraries such as Drupal check where we check for deprecated APIs and I think that is using some other PHP passing library we should be able to write something similar where we can check Yaml files for example is there an underscore access true property in there and then maybe throw a warning yeah we could do that but I think no such library exists at the moment Any more questions? Yeah, you again? Yeah, on the SPR injection example you gave you trusted the SPR API to fill the input but should we trust on such APIs or myself I always try for example if I expect an integer I have an input or check Yes Yes, so that's a very good point so the question was can we trust the Drupal database API and if I have an integer if I know it should be an integer I can just cast it to an integer for example so I think you should actually do both the API I think you need to understand what kind of stuff you can pass to the API and just never use string concatenation that's the basic rule for the database API but in principle you can trust it that's also the goal of the database API that it is a tool for the developer that is actually secure this is very important that this tool is secure and regarding the integers I think you can do it on top so if you're passing in a condition an integer you can also cast it to integer it doesn't hurt and it's clear for a reviewer also that this must always be an integer so I think it gives a lot of additional clues so I would recommend it Yep Anyone else? I have a bonus slide so yeah this was actually from another talker I gave this year also with PHP itself you can have a couple of nasty problems in this case we have an access bypass because of a typo the syntax highlighting basically gives it away already but in this case we have some security function which checks a key so the first if condition so if it's not 32 characters it returns force okay, yeah that looks correct in the second case it trims the shared key and if it equals the empty string it returns something but it's not returning force it's returning flase so this was a typical typo that some developer did and then this led to some privilege escalation vulnerability I also linked this in the notes of this talk so what would have an attacker to send in here so I can send in 32 white spaces if I send in 32 white spaces it will pass the first one and then it will go to the second one and it will be the empty string and then it will return flase and if somebody calls this function and does not strictly check what is coming out of this function I mean PHP will give a warning it will say there's some undefined constant here but PHP doesn't care that we just happily continue on and return this string and if somebody is not doing a strict comparison with the result of this function then I can elevate my access what can we do about that and we can do PHP type-hinting as a security protection so I'm a big fan of that, yeah static types everywhere so we can put in a return type-hint bool so the only thing that is allowed to come out of this function is either true or false we have to enable strict types for this to actually work through a fatal error so whenever you're working with this type-hinting I would recommend that you enable strict type for your PHP files and this is not the Drupal core standard because we're not doing it in Drupal yet but for every code that you write for example for custom modules or even for contributed modules you can say I depend on PHP 7 and I actually require this then you can do that and then it will result in a type error that the return value of the function is a string instead of a boolean and your code will not continue your site will break, right so the attacker will get a white screen but at least they didn't escalate the access and can exploit something in the site yeah so I would recommend that we should also push for that in Drupal core so that we adopt this at some point that we have more type-hints because this doesn't only review security issues in your code it also reviews type problems or other problems that you might have overlooked okay any more questions? yeah you mentioned that Drupal by default enables cross-site free-read for forms yeah and that is for authenticated yes so the question was what do we have more? what's your standpoint on for anonymous users for forms? so for cross-site the question was cross-site request folder in forms only works for authenticated users but not for anonymous users and this is a bit by design in Drupal or how Drupal understands this is this and anonymous you cannot distinguish anonymous users between each other so you don't actually know from whom the request is coming from so the security tokens that we generate in the form would be the same for each anonymous user so an attacker can just copy their security token and do something on the behalf of the anonymous user so whenever you want to protect anonymous users you either have to start a session or lock them in yep that's the reason for that because an attacker has the same access as every anonymous users to the tokens that come out of the form so they can copy it and then do something on behalf of the user and sometimes you get a security scan from an automated token that comes out as its vulnerability yes so the comment was that automated security testing tools report is as vulnerability in their reports and that's obviously not good I wonder if we have if we have some documentation on Drupal.org I'm not sure why maybe we have something yeah we should be able to look this up or we should write it should write down on Drupal.Drupal it doesn't do cross a request for free protection for anonymous users because we cannot distinguish them and because our protection with the token is based on sessions that's why we can't do it and anonymous users doesn't have a session so any more questions? alright thanks everybody