What happened to @Component?
https://github.com/spring-projects/spring-framework/blob/v5.2.3.RELEASE/spring-context/src/main/java/org/springframework/stereotype/Controller.java

Thanks for reminding

If we're treating returning as a sink we might as well flag any use of X-Forwarded-For except splitting and taking the rightmost value.

The result of return as a sink, I think the method is a util method, this usage is common.

Right, but you don't care about what is done with it subsequently? This is so general that there's almost no point checking how it is used at all.

If so, I think there is a problem as long as there is a place to call this method. If you consider what to do with the result of the client ip method, I think there will be a false negative rate. What do you think?

What uses do you actually consider dangerous, besides logging?

In addition to local output or log records, there are also user authentication based on client ip, database storage, etc., but their use is not unique, so I think it is impossible to standardize the definition.

Can we let the security-lab review it together?

E.g:

• The client ip detects whether there is login permission: UserServiceImpl
• Limit the number of logins for the client ip, which is common in scenarios such as blasting and ticketing: CaptchaService
• Insert the database as a record: LogDB
• Save it in JavaBean and insert into the database: MenuController

I can, but as this stands I would have to warn them that the query will produce really strange results:

• Directly used in logging call: positive
• Directly used in a rate-limiter: negative
• Directly returned: positive
• Returned in a field: negative

A user of this query would be very confused, because their expectation is that the way the value is used is the important thing, not trivial aspects of dataflow like is it returned from a function or used in function-local scope.

I would recommend you find some e.g. rate-limiting utility that would constitute a dangerous way to use this identifier. Alternatively you can choose to send this to the security lab with the health warning attached.

Thank you, I agree to let them know the current situation, and I also want to listen to their suggestions, so as to implement a changed query.

Perhaps provide a link regarding how you chose this list of header values?

Reference:

• github
• memorynotfound

Explain what you're intending to do here? Should this suppress some warnings completely, or just prevent repeated warnings in the same function?

Ensure that x-forwarded-for is the first to be obtained in the method of obtaining client ip.

Explain why you're using these headers for xffIsFirstGet but only X-Forwarded-For in the main query?

This is related to the xffIsFirstGet predicate.

Right, but why are you checking for all these possibilities here, but only X-Forwarded-For in UseOfLessTrustedSource? What is special about that header that means you trust it less than X-Real-IP for instance?

Yes, and X-Forwarded-For has a high usage rate.

Why are you only considering X-Forwarded-For a source of error? Isn't X-Real-IP exactly the same? I don't understand why you consider all 5 for the purposes of finding out if this is the first check in this function, but only one for the source.

X-Forwarded-For is a superposition process when used, format: client, proxy1, proxy2. X-Real-IP is a process of coverage when in use.

Right, which is why you suppose that taking the right-most comma-separated piece of X-Forwarded-For might be safer than the left-most. If X-Real-IP doesn't get used that way then it's simply always dangerous to trust, right? But at the moment we treat it as always safe?

Thank you for reviewing my pr.

You are right. But I still find it weird, regarding the questions here and above, I would like to hear the opinions of security-lab. Hope you can understand.

This is still a very very broad sink. I would much rather we looked for specific patterns we think are dangerous (for example, the startsWith condition you describe in the comment than to just exclude 7 or 8 cases you think are probably FPs and flag every other if statement.

If we look positively for suspicious usage, we can also avoid insisting the expression occurs directly with an if statement -- instead the sink is simply any startsWith(...) call, or an .equals with a non-blank right-hand side, or similar.

Factor the regex out into something like

string getIpAddressRegex() { result = ... }